[BUG]: C implementation for stats/base/dists/kumaraswamy/median has different results than the JavaScript implementation

[anandkaranubc]

Description

Pointed out by @Neerajpathak07 and @yuvi-mittal

For context, see comment

When running native tests for stats/base/dists/kumaraswamy/median, the majority of tests fail because the C implementation is generalized for a single value of a = 1.0 rather than any positive number.

The fix is to change

stdlib/lib/node_modules/@stdlib/stats/base/dists/kumaraswamy/median/src/main.c

Line 42 in 367ec2c

return 1.0 - stdlib_base_pow( 0.5, 1.0 / b );

to:

return stdlib_base_pow( 1.0 - stdlib_base_pow( 2.0, -1.0 / b ), 1.0 / a );

Related Issues

None

Questions

No.

Demo

No response

Reproduction

- Run `make install-node-addons NODE_ADDONS_PATTERN="stats/base/dists/kumaraswamy/median"`
- Run `make test TESTS_FILTER=".*/stats/base/dists/kumaraswamy/median/test/test.native.js"`

Expected Results

total:     1009
passing:   1009

Actual Results

total:     1009
passing:   27
failing:   982

Version

No response

Environments

N/A

Browser Version

No response

Node.js / npm Version

No response

Platform

No response

Checklist

• Read and understood the Code of Conduct.
• Searched for existing issues and pull requests.

[anandkaranubc]

@Neerajpathak07, do you want to proceed with a PR targeting this issue?

[kgryte]

There were a lot of issues with the PR adding the C implementation which were missed during code review. See fc5df17. Someone should take a close look at that package to determine whether everything is correct.

cc @Planeshifter

[kgryte]

Also, the tests in test.native.js still don't fully match what is in test.js.

[anandkaranubc]

Someone should take a close look at that package to determine whether everything is correct.

I will take a detailed look and update this issue. If it turns out to be a major change, I will likely create a PR to address it.

[Neerajpathak07]

@Neerajpathak07, do you want to proceed with a PR targeting this issue?

Ig @yuvi-mittal has started working upon it after I pointed out what was wrong.

[yuvi-mittal]

I have already started working on it , and i have been checking rest of the distributions too , there are few test cases more that are failing , will raise a PR soon.

[anandkaranubc]

@yuvi-mittal Any updates on this :)

[yuvi-mittal]

@anandkaranubc , hey i have been swamped with some college work ,i will get it done before eow.

[anandkaranubc]

@anandkaranubc , hey i have been swamped with some college work ,i will get it done before eow.

No rush! Just wanted to check in to see if you needed any help. Good luck with your work!

[Krishna-Sharma-g]

i changed the return statement and all test cases got passed so wanted to making a pr on it @anandkaranubc can i still pr on it

[anandkaranubc]

Hey @Krishna-Sharma-g, thanks for working on this and getting all test cases to pass! @yuvi-mittal, just checking in—are you still working on this? If not, he can open a PR.

[Krishna-Sharma-g]

okay @anandkaranubc

[yuvi-mittal]

@Krishna-Sharma-g , You can open a PR , no worries . The goal is to get work done as fast as possible.

[anandkaranubc]

@Krishna-Sharma-g, when creating a PR, please also consider the points mentioned by Athan in the discussions here and here.

Here is a reference PR to confirm whether you are on the right track: #3354

[Krishna-Sharma-g]

hello @anandkaranubc as mentioned by @kgryte that test.native.js not fully matches the test.js so there are the mismatched things in it

1.there is 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.

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

4.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.

please tell me that is that what @kgryte is talking about

[anandkaranubc]

Awesome! As long as test.native.js closely matches the test.js file with only minor modifications, it should be good. In addition to the test file changes, please carefully review this commit to ensure that the comments have been addressed. Also, we probably also need to check that the C-related files align closely with the changes made in the reference PR: #3354.

Feel free to submit a PR addressing all of this, and then I believe @Neerajpathak07 and @yuvi-mittal can also join the discussion.