Change ssb_to_psb_xyz_ICRS to not pass PINT a quantity #382
Conversation
|
Thank you for fixing this @tcromartie. I just looked at this, and I'm not convinced Enterprise should be changed in this way. It depends a bit on what is meant with the docstring. As I said in the PINT issue, PINT is inconsistent between Equatorial and Ecliptic coordinates. PINT should be accepting units, and Enterprise is providing them. But apparently the 'quantity' that works for Ecliptic coordinates is not ok. Why would the Ecliptic coordinate model accept quantities, and the Equatorial coordinate model not accept them? Perhaps the best way to make this consistent is to fix PINT, and in Enterprise do this:
Want to change the PR in that way @tcromartie ? |
enterprise/pulsar.py
Outdated
| @@ -367,7 +367,7 @@ def __init__(self, toas, model, sort=True, drop_pintpsr=True, planets=True): | |||
| "AstrometryEquatorial" if "AstrometryEquatorial" in model.components else "AstrometryEcliptic" | |||
| ) | |||
|
|
|||
| self._pos_t = model.components[which_astrometry].ssb_to_psb_xyz_ICRS(model.get_barycentric_toas(toas)).value | |||
| self._pos_t = model.components[which_astrometry].ssb_to_psb_xyz_ICRS(model.get_barycentric_toas(toas).value) | |||
There was a problem hiding this comment.
Add at the top:
from astropy.time import Time
Then change this line to:
self._pos_t = model.components[which_astrometry].ssb_to_psb_xyz_ICRS(Time(model.get_barycentric_toas(toas),format='mjd')).value
There was a problem hiding this comment.
Seems good, will do this afternoon! Sorry for the delay
There was a problem hiding this comment.
@vhaasteren I thought my second commit was failing because I didn't run black, but it looks like that induced way more changes. I didn't see anything official about this in the contributing guidelines. What are best practices here?
There was a problem hiding this comment.
Definitely need black! But can't be the latest version, I think.... Check the dev requirements? I'm on my phone and will look after I respond here...
There was a problem hiding this comment.
Thanks so much Scott! I actually ran test/lint/docs this time (with correct black ver) so hopefully it'll be ok now :)
There was a problem hiding this comment.
Yeah, some of the old committed code was from before black. So if you run black on the whole file yourself, you re-format more than just what you changed. Not very neat. But it' not wrong to put it in, let's just commit it.
See nanograv/PINT#1733