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

Update TwoPowerSphericalPotential.py #438

Merged
merged 2 commits into from
Nov 25, 2020
Merged

Update TwoPowerSphericalPotential.py #438

merged 2 commits into from
Nov 25, 2020

Conversation

gusbeane
Copy link
Contributor

Adds R2 deriv function for TwoPowerSphericalPotential

I tested against the Rforce function using some numerical derivatives, test suite should work. sample code below:

from galpy.potential import TwoPowerSphericalPotential
pot = TwoPowerSphericalPotential() 
assert (pot.R2deriv(1, 0.5) + (pot.Rforce(1+1E-8, 0.5) - pot.Rforce(1-1E-8, 0.5)) / 2E-8 ) < 1E-8
assert (pot.R2deriv(3, 0) + (pot.Rforce(3+1E-8, 0) - pot.Rforce(3-1E-8, 0)) / 2E-8 ) < 1E-8 

@jobovy
Copy link
Owner

jobovy commented Nov 24, 2020

Amazing, thanks! The travis runs take a while these days (because travis is winding down their free support for open source...), but I think the test suite will fail because TwoPowerSphericalPotential's lack of second derivatives was previously used to test some errors that should happen for potentials without second derivatives. I'll push some updates to those tests soon to fix that issue and then hopefully we can merge this quickly.

@jobovy jobovy self-assigned this Nov 24, 2020
@jobovy jobovy added this to the v1.7 milestone Nov 24, 2020
@jobovy
Copy link
Owner

jobovy commented Nov 25, 2020

My travis build is not starting, but it looks like it ran on your account and that all tests pass with the same coverage, so I'm just going to go ahead and merge this. Thanks again!

@jobovy jobovy merged commit c8029ee into jobovy:master Nov 25, 2020
@gusbeane
Copy link
Contributor Author

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants