-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add methods for querying NASA Exoplanet Archive for ephemeris #127
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
- Coverage 93.92% 91.19% -2.74%
==========================================
Files 37 41 +4
Lines 1598 2147 +549
==========================================
+ Hits 1501 1958 +457
- Misses 97 189 +92 ☔ View full report in Codecov by Sentry. |
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 like it! Just a few suggestions, mostly so that we can move towards exposing the API. This will also need test coverage before merge (but I'm guessing that's partly why its still in draft).
I'm not too worried yet about whether this will ultimately live here or need to be shared by other plugins - we're before 1.0 release and haven't committed to backwards compatibility yet, so let's just implement things and we can always change them.
lcviz/plugins/ephemeris/ephemeris.py
Outdated
if self.query_name: | ||
self._query_result = self.nasa_exoplanet_archive.query_object( | ||
self.query_name, table='pscomppars' | ||
) | ||
|
||
elif None not in (self.query_ra, self.query_dec): | ||
coord = SkyCoord(ra=self.query_ra, dec=self.query_dec, unit=u.deg) | ||
self._query_result = self.nasa_exoplanet_archive.query_region( | ||
coord, self.query_radius * u.arcsec, | ||
table='pscomppars' | ||
) | ||
|
||
else: | ||
# no metadata found for RA, Dec, or object name | ||
return None |
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.
can we either have the UI reflect this logic (only show coordinate inputs if the name entry is empty or something) or just require inputing a name (is there ever a case where someone would actually want to use this by coordinates? I guess maybe to search for background contamination?)?
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.
My main motivation for the backup here is that anyone can generate a file with whatever they want in the "OBJECT" card, and every star has aliases. If the name that they use can't be resolved by the archive, the coordinates provide another way.
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.
That's fine, I just think we need to think of a UI/UX that helps make that clear. So either a dropdown to choose whether to query by identifier or coords, or have the coords hide when the identifier is not blank might help to make that feel intuitive without having to look in the docs.
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 think eventually we may want to move this up or even into the ephemeris component... component, but this is good for now until we decide what else we want to support for querying (other catalogs, loading from metadata, etc).
Just one comment on the diff and then we can always iterate further later before an official 1.0 release - this definitely is useful to get in ASAP!
@kecnry I've renamed |
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.
Functionality looks good to me now, just a few requests for API docs, but otherwise I think this is good to go in, thanks!
lcviz/plugins/ephemeris/ephemeris.py
Outdated
'adopt_period_at_max_power', 'query_for_ephemeris', | ||
'query_result', 'adopt_from_catalog', 'create_ephemeris_from_query' |
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.
query_for_ephemeris
, query_result
, and create_ephemeris_from_query
need to be added to the API docs.
Co-authored-by: Kyle Conroy <[email protected]>
Demo