Skip to content
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.

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

test demo-ing RT#107326 problem #24

Closed
wants to merge 1 commit into from
Closed

test demo-ing RT#107326 problem #24

wants to merge 1 commit into from

Conversation

mohawk2
Copy link
Member

@mohawk2 mohawk2 commented May 30, 2016

This demonstrates the issue. It does not fix it (yet). This is quite complex as while the GSL function has an unsigned parameter, the PDL code is probably just silently casting the negative number to a large positive one. I don't know enough now about the PDL framework to constrain inputs as would be needed here.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 74.951% when pulling 71fb3d0 on rt-107326 into 19d0dfe on master.

@maggiexyz
Copy link
Contributor

Thanks for adding the test cases! The Travis build didn't seem to have run the cdf test. It should have failed given the new test. Closing the PR without merging for now as it'll break the master build unless the test is fixed or ignored.

@maggiexyz maggiexyz closed this Jun 1, 2016
@mohawk2
Copy link
Member Author

mohawk2 commented Jun 1, 2016

I'm going to reopen since this is still a live issue, and this being open indicates that.

@mohawk2 mohawk2 reopened this Jun 1, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 74.951% when pulling 71fb3d0 on rt-107326 into 19d0dfe on master.

@mohawk2 mohawk2 force-pushed the master branch 9 times, most recently from 5baa8ad to 4052439 Compare December 19, 2020 22:39
@mohawk2
Copy link
Member Author

mohawk2 commented Jul 26, 2021

Most immediately, this is no longer an issue for this repo since the CDF functions are now in main PDL.

Going slightly deeper, this is an example of PDL working as designed; the inputs for functions including gsl_cdf_hypergeometric_P which take unsigned values are having the inputs coerced to unsigned values. For -1 this gets changed to 65535, which obviously is not what the user expects and is quite unhelpful.

For reasons of backward compatibility, we can't change the behaviour for negative inputs on conversion to unsigned. Therefore, unfortunately there's nothing to be done in this case. Sorry to have taken so long to dig into this!

@mohawk2 mohawk2 closed this Jul 26, 2021
@mohawk2 mohawk2 deleted the rt-107326 branch July 26, 2021 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants