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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

keyword parameter t in galpy.orbit.Orbits.Orbit._call_internal will be ignored in some cases #565

Closed
azz147 opened this issue Apr 21, 2023 · 3 comments 路 Fixed by #569
Closed

Comments

@azz147
Copy link
Contributor

azz147 commented Apr 21, 2023

馃悰 Bug

The below line will cause that passing single keyword parameter t will be ignored because len(args)==0 is true in that case

if len(args) == 0 or (not hasattr(self,'t') and args[0] == 0. ):

Reproducible example

from astropy import units
from galpy.potential import MWPotential2014, ChandrasekharDynamicalFrictionForce
from galpy.orbit import Orbit
import numpy
o= Orbit.from_name('LMC')
ts= numpy.linspace(0.,-10.,1001)*units.Gyr
o.integrate(ts,MWPotential2014)
print(o.SkyCoord(t=0).ra ,o.SkyCoord(t=-100).ra)
# 78d46m12s 78d46m12s
print(o.SkyCoord(0).ra,o.SkyCoord(-100).ra)
# 78d46m12s 174d18m00.10529969s

Expected behavior

I expect the below lines should have the same output or the document states the t parameter will be ignored in this case.

print(o.SkyCoord(t=0).ra ,o.SkyCoord(t=-100).ra)
print(o.SkyCoord(0).ra,o.SkyCoord(-100).ra)
@jobovy
Copy link
Owner

jobovy commented Apr 21, 2023

Yes, it's true that this doesn't work. Perhaps the documentation could be clearer, but t is supposed to be given as an argument, not a keyword argument:

https://docs.galpy.org/en/latest/reference/orbitskycoord.html

A quick fix would be to insert

if len(args) == 0 and 't' in kwargs:
    args= [kwargs.get('t')]

just before

if len(args) == 0 or (not hasattr(self,'t') and args[0] == 0. ):

Care to open a PR? Would also need a test in tests/test_orbit.py.

@azz147
Copy link
Contributor Author

azz147 commented May 9, 2023

Sorry for the late reply. I've created a PR #569 about this.
I use args= [kwargs.pop('t')] to mimic the case where t is passed as args.

I tried to run all the tests in tests/test_orbit.py but it seems even with the unchanged code some tests still fail. I am not sure whether this is expected.

For example, use commit d6234fd and run

python -m pytest -v tests/test_orbit.py::test_physical_output

I will get

FAILED tests/test_orbit.py::test_physical_output - AttributeError: 'Orbit' object has no attribute '_aA_Or'

@jobovy
Copy link
Owner

jobovy commented May 9, 2023

Thanks, the PR looks good!

Don't worry about that error, I think it's unrelated to the change (and all the CI tests pass). I think I've encountered this locally before as well and don't quite remember what the issue was. I think it might be that you don't have the C extensions compiled and then the actionAngle method used in that test falls back onto a Python version that does not have all features, resulting in missing attributes.

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

Successfully merging a pull request may close this issue.

2 participants