-
-
Couldn't load subscription status.
- Fork 424
Fix docstring for eccentricity and perihelion time #3444
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
base: main
Are you sure you want to change the base?
Conversation
For unbound objects on hyperbolic orbits e > 1. The format for the perihelion time for comets was listed as a string, but MOST actually requires the time as a Julian Date (float). If you provide a string here the query will fail.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3444 +/- ##
=======================================
Coverage 70.72% 70.72%
=======================================
Files 232 232
Lines 20041 20041
=======================================
Hits 14174 14174
Misses 5867 5867 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides making the docs consistent for both methods and the narrative docs; I wonder if this should be fixed in code and in fact we could accept an astropy Time object or a string that can be parsed into an astropy Time?
(The code fixes are definitely beyond the scope here, but could be reported in a separate issue)
| perih_time : float or None | ||
| Perihelion time (JD). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perih_time is also a parameter for query_image; is this an issue there, too?
And it is also mentioned in the narrative docs file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I missed that. It should JD as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through all the parameters and found a few other errors. I fixed all that I found. 0497d52
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update these in the narrative docs file? docs/ipac/irsa/most.rst
|
I think the formats for the manual input option in MOST were set so that it's easy to copy-paste the the orbital parameters from the Minor Planets Center (e.g., https://data.minorplanetcenter.net/db_search/show_object?utf8=%E2%9C%93&object_id=2I). So I don't think switching the date format would improve usability, unless there were also a plan to set up functionality to query the MPC database from astroquery as well. |
|
I now see that there is already an MPC module, so maybe this could be a future goal to make these two talk to each other in a convenient way. |
For unbound objects on hyperbolic orbits e > 1.
The format for the perihelion time for comets was listed as a string, but MOST actually requires the time as a Julian Date (float). If you provide a string here the query will fail.