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

Add elliptic integral functions to safe api #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Yatekii
Copy link

@Yatekii Yatekii commented Mar 15, 2019

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.

@Yatekii
Copy link
Author

Yatekii commented Mar 16, 2019

Could it be that your version of CEPHES is outdated? Is this intentional? Scipys is modified and thus certain functions are different.

@vks
Copy link
Owner

vks commented Mar 18, 2019

Could it be that your version of CEPHES is outdated?

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.

Copy link
Owner

@vks vks left a comment

Choose a reason for hiding this comment

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

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

@vks
Copy link
Owner

vks commented Mar 18, 2019

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.

@Yatekii
Copy link
Author

Yatekii commented Mar 18, 2019

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.

@vks
Copy link
Owner

vks commented Mar 18, 2019

I am talking about his change: https://github.com/scipy/scipy/blob/master/scipy/special/cephes/ellpe.c#L98 This is significant!

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.

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.

2 participants