Add elliptic integral functions to safe api#10
Conversation
|
Could it be that your version of CEPHES is outdated? Is this intentional? Scipys is modified and thus certain functions are different. |
I'm using Cephers version 2014-10-04 from Stephen Moshier's website. In the meantime, the double-precision version was updated, but the single-precision version was not. I don't know which version SciPy uses or whether they forked it. I know that SciPy uses different names, and we are using them for the high-level bindings as well. Edit: I just checked the changes, they do not affect the Rust bindings. |
There was a problem hiding this comment.
Please remove the Cargo.lock file from the commit.
How did you do tests? Do reference calculations in python for example?
I can add those if needed!
Yes, could you please add a simple test for each of the new functions? You can have a look at the other tests, typically they only evaluate the function once and compare it it to the reference value obtained with SciPy. (This should be enough, because the Cephes functions are already well tested. Having some test is nice, because otherwise we won't notice when there is a linking issue.)
|
Note that the functions you seem to missing from SciPy are implemented in terms of the other functions provided by Cephes: Feel free to add them as high-level functions. |
|
I am talking about his change: https://github.com/scipy/scipy/blob/master/scipy/special/cephes/ellpe.c#L98 This is significant! The question is whether the scipy API or the API of your CEPHES version should be used ;) Ok, I will add scipy tests. |
Thanks! Looks like a change specific to SciPy's fork. If you want, you can change the high-level implementation to accept arguments larger than 1. Just make sure to document the valid arguments in the docstring. |
I just added the functions I need for now.
How did you do tests? Do reference calculations in python for example?
I can add those if needed!
If there is something I am missing, let me know, I'll update this PR.