New module to query Lowell Observatory astorbDB database#3203
New module to query Lowell Observatory astorbDB database#3203hhsieh00 wants to merge 171 commits intoastropy:mainfrom
Conversation
mkelley
left a comment
mkelley
left a comment
There was a problem hiding this comment.
Looks good and very useful in general. Some initial comments and questions to address while you continue testing.
|
Thanks @mkelley for the review. I haven't looked into it in any meaningful way yet, but I notice that there are a lot of comments on async stuff. We should remove/rework the template module as a lot has changed. E.g. there is a preference of supporting actual async/sync API and not just with the early template hackary. So I would suggest to so that, too, if the service support async, then opt into that with a keyword argument, but don't duplicate the methods with the decorator. or one PR that did the keyword approach: #3201 or the ESA modules also have a |
|
I'm not sure why the checks didn't run. @hhsieh00 You might want to try to |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3203 +/- ##
==========================================
+ Coverage 71.72% 72.03% +0.30%
==========================================
Files 235 238 +3
Lines 20253 20487 +234
==========================================
+ Hits 14527 14757 +230
- Misses 5726 5730 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| Returns | ||
| ------- | ||
| res : A dictionary holding available AstInfo data |
There was a problem hiding this comment.
@bsipocz is it OK that this particular _async method returns a dictionary of responses? _parse_result appropriately handles it, so the class is internally consistent.
There was a problem hiding this comment.
I would say drop duplicating the async/sync methods approach
| def _process_data_elements(self, src): | ||
| """ | ||
| internal routine to process raw data in Dict format, must | ||
| be able to work recursively |
There was a problem hiding this comment.
Maybe some additional comments could be added here to help with code maintenance. It seems to me that this is primarily about assigning units to returned values. Is there a place in the astorb documentation where the units were defined?
There was a problem hiding this comment.
added some more specific comments
|
I can reduce some of the test data. Multiple objects are necessary since some have null results (e.g., main-belt asteroids don't have escape route data, while NEOs don't have family data, and relatively newly discovered asteroids don't have lightcurve, taxonomic, or albedo data), but I can reduce it to less than what's there currently. |
query code only for now; tests not written yet
Fixing some codestyle issues in core.py
fixing more codestyle issues in core.py
First complete draft of the astorbdb class including tests and documentation
removed get_raw_response lines, changed names of dynamical_family and escape_routes methods, changed OrderedDict output to regular dictionaries. Also re-ordered methods in alphabetical order. Other minor edits.
fixed dynamical_family and escape_route function names in documentation; also added astorbdb.rst to various toc files
query code only for now; tests not written yet
Fixing some codestyle issues in core.py
fixing more codestyle issues in core.py
First complete draft of the astorbdb class including tests and documentation
removed get_raw_response lines, changed names of dynamical_family and escape_routes methods, changed OrderedDict output to regular dictionaries. Also re-ordered methods in alphabetical order. Other minor edits.
query code only for now; tests not written yet
Fixing some codestyle issues in core.py
fixing more codestyle issues in core.py
This reverts commit 8920bf8.
This reverts commit bf119b2.
This reverts commit d56b251.
This reverts commit b51ff22.
This reverts commit 31581a6.
This reverts commit 543dea9.
This reverts commit 8bb7a92.
This reverts commit e590e4f.
This reverts commit 49700d0.
This reverts commit ca91720.
This reverts commit 5e13d19.
This reverts commit 3d9d148.
This reverts commit 70c8297.
This reverts commit f57df66.
This reverts commit 4b17fb8.
This reverts commit 756d0dc.
This reverts commit 646d3a2.
This reverts commit 804a662.
This reverts commit 1adca45.
This reverts commit 84b5feb.
This reverts commit dccfb7e.
This reverts commit e535c6d.
hhsieh00
force-pushed
the
3129-add-lowell-astorbdb-query-module
branch
from
92a6f86 to
13be374
Compare
removed automodapi code from solarsystem version of lowellmps.rst to match other sections (e.g., jpl, imcce)
mkelley
left a comment
mkelley
left a comment
There was a problem hiding this comment.
Just one comment on the documentation.
| an interface to the `Asteroid Information | ||
| <https://asteroid.lowell.edu/astinfo/>`_ (AstInfo) | ||
| tool provided as part of | ||
| the Lowell Minor Planet Services which utilize the `astorbDB |
There was a problem hiding this comment.
I didn't really understand that there was a publication related to this until I followed the link. What about re-writing this link in citation style? Something like, "astorbDB database (Moskovitz et al. 2022 <url>_)"?
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
Thank you @hhsieh00 for this PR. I have a a large collection of comments, but this feels to be rather close to be merge able. The big picture items:
- I think the module could be named in a bit more descriptive way
- there are way too many commits with a lot of revertions. Those should preferably be squashed into the commits they are reverting.
- I have commented on the async-sync question already, there should not be a duplicate of methods any more, no usage of the decorator and if the service supports real async, then control it with a kwarg, like
async_job=True - I feel the API can be a bit more descriptive or better fit into the method naming approaches we use in the other modules (e.g. not just a noun but an active verb+noun, e.g.
query_albedos-- I firmly believe this will help users in the long run)
Again, thank you so much for working on this module, I hope while the comments are numerous they won't be too complicated to be addressed.
| New Tools and Services | ||
| ---------------------- | ||
|
|
||
| lowellmps |
There was a problem hiding this comment.
I would consider adding an underscore in the name, that would ease readibility and we have precedent for it e.g. esa.xmm_newton or ipac.nexsci.nasa_exoplanet_archive
There was a problem hiding this comment.
@bsipocz I would be in favor of this. With the nexsci name in mind in particular, what do you think about expanding it a bit more to lowell_minor_planet_services? "MPS" is not really an abbreviation in common usage that I'm aware of, so spelling it out completely would be the best for readability in my opinion, at the expense of compactness.
| LOWELLMPS | ||
| ------------------------- | ||
|
|
||
| :author: Henry Hsieh (hhsieh@gmail.com) |
There was a problem hiding this comment.
maybe add a 1-2 sentence description here, otherwise loose this docstring.
Also, note that we are cleaning out this author information -- the git history should hold that information much better than these files.
| lowellmps | ||
| ^^^^^^^^^ | ||
|
|
||
| - module added to query the Lowell Minor Planet Services database [#3203] |
There was a problem hiding this comment.
only the AstInfo tool was added, so be explicit about it
| __all__ = ['AstInfo', 'AstInfoClass'] | ||
|
|
||
|
|
||
| @async_to_sync |
There was a problem hiding this comment.
Don't use this any more; if async is supported by the server add support for it via a kwarg.
| # actual query uri | ||
| _uri = None | ||
|
|
||
| def albedos_async(self, object_name, *, |
| an interface to the `Asteroid Information | ||
| <https://asteroid.lowell.edu/astinfo/>`_ (AstInfo) | ||
| tool provided as part of | ||
| the Lowell Minor Planet Services which utilize the `astorbDB |
| >>> from astroquery.lowellmps import AstInfo | ||
| >>> albedos = AstInfo.albedos('656') | ||
| >>> print(albedos) # doctest: +IGNORE_OUTPUT | ||
| [{'albedo': 0.065, 'albedo_error_lower': -0.002, 'albedo_error_upper': 0.002, 'citation_bibcode': '2011PASJ...63.1117U', 'citation_url': 'https://darts.isas.jaxa.jp/astro/akari/catalogue/AcuA.html', 'count_bands_detection': 2, 'diameter': <Quantity 54.32 km>, 'diameter_error_lower': <Quantity -0.77 km>, 'diameter_error_upper': <Quantity 0.77 km>, 'eta': 0.82, 'eta_error_lower': None, 'eta_error_upper': None, 'measurement_techniques': ['Mid IR photometry'], 'observations_total': 11, 'survey_name': 'Usui et al. (2011)'}, {'albedo': 0.0625, 'albedo_error_lower': -0.015, 'albedo_error_upper': 0.015, 'citation_bibcode': '2004PDSS...12.....T', 'citation_url': 'http://sbn.psi.edu/pds/asteroid/IRAS_A_FPA_3_RDR_IMPS_V6_0.zip', 'count_bands_detection': 3, 'diameter': <Quantity 53.17 km>, 'diameter_error_lower': <Quantity -5.5 km>, 'diameter_error_upper': <Quantity 5.5 km>, 'eta': 0.756, 'eta_error_lower': -1, 'eta_error_upper': 1, 'measurement_techniques': ['Mid IR photometry'], 'observations_total': 18, 'survey_name': 'Infrared Astronomical Satellite (IRAS)'}, {'albedo': 0.075, 'albedo_error_lower': -0.011, 'albedo_error_upper': 0.011, 'citation_bibcode': '2019PDSS..251.....M', 'citation_url': 'http://sbnarchive.psi.edu/pds4/non_mission/neowise_diameters_albedos_V2_0.zip', 'count_bands_detection': 4, 'diameter': <Quantity 48.471 km>, 'diameter_error_lower': <Quantity -0.535 km>, 'diameter_error_upper': <Quantity 0.535 km>, 'eta': 1.033, 'eta_error_lower': -0.021, 'eta_error_upper': 0.021, 'measurement_techniques': ['Mid IR photometry'], 'observations_total': 58, 'survey_name': 'NEOWISE'}, {'albedo': 0.045, 'albedo_error_lower': -0.005, 'albedo_error_upper': 0.005, 'citation_bibcode': '2019PDSS..251.....M', 'citation_url': 'http://sbnarchive.psi.edu/pds4/non_mission/neowise_diameters_albedos_V2_0.zip', 'count_bands_detection': 4, 'diameter': <Quantity 62.604 km>, 'diameter_error_lower': <Quantity -0.512 km>, 'diameter_error_upper': <Quantity 0.512 km>, 'eta': 1.458, 'eta_error_lower': -0.025, 'eta_error_upper': 0.025, 'measurement_techniques': ['Mid IR photometry'], 'observations_total': 46, 'survey_name': 'NEOWISE'}, {'albedo': 0.092, 'albedo_error_lower': -0.041, 'albedo_error_upper': 0.041, 'citation_bibcode': '2019PDSS..251.....M', 'citation_url': 'http://sbnarchive.psi.edu/pds4/non_mission/neowise_diameters_albedos_V2_0.zip', 'count_bands_detection': 2, 'diameter': <Quantity 38.214 km>, 'diameter_error_lower': <Quantity -10.329 km>, 'diameter_error_upper': <Quantity 10.329 km>, 'eta': 0.95, 'eta_error_lower': -0.2, 'eta_error_upper': 0.2, 'measurement_techniques': ['Mid IR photometry'], 'observations_total': 16, 'survey_name': 'NEOWISE'}, {'albedo': 0.069, 'albedo_error_lower': -0.034, 'albedo_error_upper': 0.034, 'citation_bibcode': '2019PDSS..251.....M', 'citation_url': 'http://sbnarchive.psi.edu/pds4/non_mission/neowise_diameters_albedos_V2_0.zip', 'count_bands_detection': 2, 'diameter': <Quantity 45.005 km>, 'diameter_error_lower': <Quantity -15.483 km>, 'diameter_error_upper': <Quantity 15.483 km>, 'eta': 0.95, 'eta_error_lower': -0.2, 'eta_error_upper': 0.2, 'measurement_techniques': ['Mid IR photometry'], 'observations_total': 26, 'survey_name': 'NEOWISE'}, {'albedo': 0.083, 'albedo_error_lower': -0.054, 'albedo_error_upper': 0.054, 'citation_bibcode': '2019PDSS..251.....M', 'citation_url': 'http://sbnarchive.psi.edu/pds4/non_mission/neowise_diameters_albedos_V2_0.zip', 'count_bands_detection': 2, 'diameter': <Quantity 40.12 km>, 'diameter_error_lower': <Quantity -14.369 km>, 'diameter_error_upper': <Quantity 14.369 km>, 'eta': 0.95, 'eta_error_lower': -0.2, 'eta_error_upper': 0.2, 'measurement_techniques': ['Mid IR photometry'], 'observations_total': 22, 'survey_name': 'NEOWISE'}] |
There was a problem hiding this comment.
I wonder if adding some whitespaces would make this a bit better readable? After all we're already ignoring the output
| Output | ||
| ====== |
There was a problem hiding this comment.
there are already examples above with outputs, so I would find it helpful if this heading would be a bit more detailed
| @@ -0,0 +1,250 @@ | |||
| .. _astroquery.lowellmps: | |||
|
|
|||
There was a problem hiding this comment.
If you add this, all the remote-data directives can be removed from below
| .. as all code examples require remote-data access, thus only using this one at the very top | |
| .. doctest-remote-data-all:: | |
| database. | ||
|
|
||
| The development of this submodule is funded through NASA PDART | ||
| Grant No. 80NSSC18K0987 to the `sbpy project <https://sbpy.org>`_. |
There was a problem hiding this comment.
Is this still the correct grant number or just a copy paste error from the other solarsystem modules
There was a problem hiding this comment.
Ah, yes, @hhsieh00 that should be 80NSSC22K0143
3 participants
This is a new module for querying the Lowell Observatory astorbDB database (see issue #3129), specifically using REST APIs used to run their AstInfo tool (https://asteroid.lowell.edu/astinfo/). The AstInfo class in this module specifically retrieves designation, orbital element, orbit, albedo, color, taxonomy, lightcurve, dynamical family, and escape route data for asteroids catalogued in the Lowell astorbDB database.