Allow Stars to have epochs of position other than T0. #166
Conversation
skyfield/starlib.py
Outdated
| @@ -40,7 +40,7 @@ class Star(object): | |||
|
|
|||
| def __init__(self, ra=None, dec=None, ra_hours=None, dec_degrees=None, | |||
| ra_mas_per_year=0.0, dec_mas_per_year=0.0, | |||
| parallax_mas=0.0, radial_km_per_s=0.0, names=()): | |||
| parallax_mas=0.0, radial_km_per_s=0.0, jd_of_position=T0, names=()): | |||
There was a problem hiding this comment.
What would you think of naming this epoch to agree with the terminology of the Hipparcos catalog?
There was a problem hiding this comment.
I chose this version because it makes clear the units of the input value, in the same way the _km_per_s suffixes on some of the arguments to this function do.
There was a problem hiding this comment.
- Maybe
epoch_jd, then? - Drat, now I'm thinking of the much bigger question of whether this should take a
Timeobject :) It doesn't need to, right? There's other places in the code already where the interface just sticks to a plain jd?
There was a problem hiding this comment.
(Just checking back in: before we land, do you have an opinion on the spelling here, and the question of allowing a Time vs just a float?)
There was a problem hiding this comment.
Thanks for the nudge. I don't like epoch as the root word of the name for this parameter since in my opinion it's more vague to the uninitiated — but I'm not sure how to make that choice anything more than a matter of opinion. In which case I think "package owner's opinion wins" is the only sensible resolution process ...
And, yeahhhh, if there are Time objects, it should probably take a Time object. It was sheer laziness on my part not to look into how to make that work. I'd presume it's pretty easy — just convert it to a JD and save that?
There was a problem hiding this comment.
Let's try epoch and I'll see what it looks like as I try using it after it lands! If they pass a float, then just keep it untouched; but if they pass a time object from Skyfield's timelib, I think you can ask it for its .tt attribute to get the float you need. Thanks!
There was a problem hiding this comment.
The epoch value is subtracted from a TDB time. Should I ask for the Time's .tdb attribute instead? (I am wondering if maybe asking for TDB opens up a can of worms.)
There was a problem hiding this comment.
Oh! Yes, then .tdb is the correct attribute, thanks!
skyfield/documentation/stars.rst
Outdated
| barnard = Star(ra_hours=(17, 57, 48.49803), | ||
| dec_degrees=(4, 41, 36.2072), | ||
| ra_mas_per_year=-798.71, | ||
| dec_mas_per_year=+10337.77, | ||
| parallax_mas=545.4, | ||
| radial_km_per_s=-110.6) | ||
| radial_km_per_s=-110.6, | ||
| jd_of_position=T0) |
There was a problem hiding this comment.
I suspect we should expand this to two separate examples, one without, and one with the new keyword, since many users cut-and-paste without reading the text. But I can do that after this lands if you don't get around to it!
There was a problem hiding this comment.
As per Twitter conversion from yesterday and today, I suspect? I mostly avoided this since an example that was almost entirely redundant seemed a bit lame, but I didn't want to go to the work of coming up with and validating another example :-)
Cf [the GitHub pull request](skyfielders#166): - Rename the parameter to just `epoch` - Allow Time objects to be provided, pulling out their TDB values.
|
OK, this PR should now include the asked-for changes, as I understand them. |
skyfield/starlib.py
Outdated
| @@ -40,7 +41,7 @@ class Star(object): | |||
|
|
|||
| def __init__(self, ra=None, dec=None, ra_hours=None, dec_degrees=None, | |||
| ra_mas_per_year=0.0, dec_mas_per_year=0.0, | |||
| parallax_mas=0.0, radial_km_per_s=0.0, names=()): | |||
| parallax_mas=0.0, radial_km_per_s=0.0, epoch=T0, names=()): | |||
There was a problem hiding this comment.
Oh, there's one final adjustment needed — I should have noticed before: epoch needs to go last, else any code that folks have written that supplies plain non-keyword arguments for a Star will break if we take the argument that was once names and use that same slot for epoch instead. Try moving epoch= to the end so that existing calls keep working. Thanks!
|
Thanks! |
Pursuant to issue #125. I have documented the new keyword but not added test coverage.