Skip to content

fix: update C implementation for stats/base/dists/kumaraswamy/median #5452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Mar 1, 2025
Merged

fix: update C implementation for stats/base/dists/kumaraswamy/median #5452

merged 9 commits into from
Mar 1, 2025

Conversation

Krishna-Sharma-g
Copy link
Contributor

@Krishna-Sharma-g Krishna-Sharma-g commented Feb 25, 2025 •
edited
Loading

Resolves #5047 .

Description

#5047

What is the purpose of this pull request?

This pull request:

fixes the bug of failing testcases in the kumarswamy/median/src/main.c and also improved and matched the test.native.js and test.js

    1. Fixed the incorrectly named test that was referring to "gamma" instead of "a" by completely replacing it with the proper test for nonpositive "a" parameter from test.js.

  • 2.Added the missing test for nonpositive "b" parameter, copying the test cases from test.js.

  • 3.Ensured all edge cases are properly tested with the same combinations of parameter values as in test.js.
    Maintained the native-specific parts like the tryRequire setup and opts variable for test skipping.

Related Issues

Does this pull request have any related issues?

no

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

image

Checklist

Please ensure the following tasks are completed before submitting this pull request.

@stdlib-js/reviewers

@Krishna-Sharma-g
fix: correct median formula in kumaraswamy distribution
c62f2f7 
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: missing_dependencies
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---

---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
  - task: run_javascript_examples
    status: na
  - task: run_c_examples
    status: na
  - task: run_cpp_examples
    status: na
  - task: run_javascript_readme_examples
    status: na
  - task: run_c_benchmarks
    status: na
  - task: run_cpp_benchmarks
    status: na
  - task: run_fortran_benchmarks
    status: na
  - task: run_javascript_benchmarks
    status: na
  - task: run_julia_benchmarks
    status: na
  - task: run_python_benchmarks
    status: na
  - task: run_r_benchmarks
    status: na
  - task: run_javascript_tests
    status: na
---
@Krishna-Sharma-g
Update test.native.js
ac0c3bc 
Signed-off-by: Gautam sharma <[email protected]>
@stdlib-bot stdlib-bot added Statistics Issue or pull request related to statistical functionality. First-time Contributor A pull request from a contributor who has never previously committed to the project repository. Needs Review A pull request which needs code review. labels Feb 25, 2025
@stdlib-bot
Copy link
Contributor

Hello! Thank you for your contribution to stdlib.

We noticed that the contributing guidelines acknowledgment is missing from your pull request. Here's what you need to do:

  1. Please read our contributing guidelines.

  2. Update your pull request description to include this checked box:

    - [x] Read, understood, and followed the [contributing guidelines](https://github.com/stdlib-js/stdlib/blob/develop/CONTRIBUTING.md)

This acknowledgment confirms that you've read the guidelines, which include:

  • The developer's certificate of origin
  • Your agreement to license your contributions under the project's terms

We can't review or accept contributions without this acknowledgment.

Thank you for your understanding and cooperation. We look forward to reviewing your contribution!

stdlib-bot
stdlib-bot reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@stdlib-bot stdlib-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Hi there! 👋

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

@stdlib-bot
Copy link
Contributor

stdlib-bot commented Feb 25, 2025 •
edited
Loading

Coverage Report

Package Statements Branches Functions Lines
stats/base/dists/kumaraswamy/median $\color{green}196/196$
$\color{green}+100.00\%$ 		$\color{green}10/10$
$\color{green}+100.00\%$ 		$\color{green}2/2$
$\color{green}+100.00\%$ 		$\color{green}196/196$
$\color{green}+100.00\%$

The above coverage report was generated for the changes in this PR.

@Krishna-Sharma-g
Copy link
Contributor Author

hello @anandkaranubc please review once

@anandkaranubc
Copy link
Contributor

@Krishna-Sharma-g, thanks for opening this PR. As I mentioned earlier in the issue, there are many more changes required for the current package stats/base/dists/kumaraswamy/median.

I have already created a commit here that will guide you on what other changes need to be made. You can just copy all the changes from there and update this PR. Cheers!

@anandkaranubc
Copy link
Contributor

Can you also change the title to:

fix: update C implementation for stats/base/dists/kumaraswamy/median

Note that there are backticks around stats/base/dists/kumaraswamy/median

@anandkaranubc anandkaranubc added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Feb 26, 2025
@anandkaranubc
Copy link
Contributor

I also noticed that your current PR is based off your develop branch. While you can keep it like this for the current PR, I would suggest creating a new branch each time for future changes/PRs.

@Krishna-Sharma-g @anandkaranubc
Update lib/node_modules/@stdlib/stats/base/dists/kumaraswamy/median/s…
d3c7315 
…rc/main.c

Co-authored-by: Karan Anand <[email protected]>
Signed-off-by: Gautam sharma <[email protected]>
@Krishna-Sharma-g Krishna-Sharma-g changed the title [BUG]: C implementation for stats/base/dists/kumaraswamy/median has different results than the JavaScript implementation #5047 fix: update C implementation for stats/base/dists/kumaraswamy/median Feb 26, 2025
@Krishna-Sharma-g
Copy link
Contributor Author

@Krishna-Sharma-g, thanks for opening this PR. As I mentioned earlier in the issue, there are many more changes required for the current package stats/base/dists/kumaraswamy/median.

I have already created a commit here that will guide you on what other changes need to be made. You can just copy all the changes from there and update this PR. Cheers!

okay @anandkaranubc thankyou for that

@Krishna-Sharma-g
Copy link
Contributor Author

Can you also change the title to:

fix: update C implementation for stats/base/dists/kumaraswamy/median

Note that there are backticks around stats/base/dists/kumaraswamy/median

done !

@Krishna-Sharma-g Krishna-Sharma-g changed the title fix: update C implementation for stats/base/dists/kumaraswamy/median fix: update C implementation for stats/base/dists/kumaraswamy/median Feb 26, 2025
@Krishna-Sharma-g
Copy link
Contributor Author

I also noticed that your current PR is based off your develop branch. While you can keep it like this for the current PR, I would suggest creating a new branch each time for future changes/PRs.

okay next time i will be aware of that

@Krishna-Sharma-g
Copy link
Contributor Author

@anandkaranubc does this pr need any more changes or it is ready to merge

@anandkaranubc
Copy link
Contributor

@anandkaranubc does this pr need any more changes or it is ready to merge

@Krishna-Sharma-g Everything, that was made in this commit here

@Krishna-Sharma-g
Copy link
Contributor Author

@anandkaranubc oo sorry i missed that

@Krishna-Sharma-g
Copy link
Contributor Author

hello @anandkaranubc i synced the file and due to that it commited the
Merge branch 'stdlib-js:develop' into develop so do i have to do something or it is okay

@anandkaranubc
Copy link
Contributor

anandkaranubc commented Feb 26, 2025 •
edited
Loading

There aren't any "file" changes in the PR, so there's nothing to worry about.

@Krishna-Sharma-g
Copy link
Contributor Author

@anandkaranubc changes done !

@anandkaranubc
Copy link
Contributor

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Feb 27, 2025
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Feb 27, 2025
@Krishna-Sharma-g
Copy link
Contributor Author

@anandkaranubc on it

@Krishna-Sharma-g
Update benchmark.c
04c1793 
Signed-off-by: Gautam sharma <[email protected]>
@anandkaranubc
Copy link
Contributor

@kgryte, I think this is ready for merge. As you mentioned, there are many more changes than initially stated in the issue. Hopefully, we've covered them all. Thanks!

@anandkaranubc anandkaranubc requested a review from kgryte February 27, 2025 13:40
@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Feb 27, 2025
@anandkaranubc anandkaranubc removed the Needs Changes Pull request which needs changes before being merged. label Feb 27, 2025
Planeshifter
Planeshifter previously approved these changes Mar 1, 2025
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Krishna-Sharma-g for your contribution and @anandkaranubc for initial review! Will merge PR shortly.

@Planeshifter Planeshifter added the Ready To Merge A pull request which is ready to be merged. label Mar 1, 2025
@stdlib-bot stdlib-bot removed the Needs Review A pull request which needs code review. label Mar 1, 2025
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Mar 1, 2025 •
edited by Planeshifter
Loading

PR Commit Message

fix: update C implementation for `stats/base/dists/kumaraswamy/median`

PR-URL: https://github.com/stdlib-js/stdlib/pull/5452
Closes: https://github.com/stdlib-js/stdlib/issues/5047

Co-authored-by: Karan Anand <[email protected]>
Co-authored-by: stdlib-bot <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Karan Anand <[email protected]>
Signed-off-by: Gautam sharma <[email protected]>

Please review the above commit message and make any necessary adjustments.

@Planeshifter Planeshifter merged commit eaccd6c into stdlib-js:develop Mar 1, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anandkaranubc anandkaranubc anandkaranubc requested changes

@Planeshifter Planeshifter Planeshifter approved these changes

@stdlib-bot stdlib-bot Awaiting requested review from stdlib-bot

@kgryte kgryte Awaiting requested review from kgryte

Assignees
No one assigned
Labels
First-time Contributor A pull request from a contributor who has never previously committed to the project repository. Ready To Merge A pull request which is ready to be merged. Statistics Issue or pull request related to statistical functionality.
Projects
None yet
Milestone
No milestone
4 participants
@Krishna-Sharma-g @stdlib-bot @anandkaranubc @Planeshifter