Add ability to ingest Solar System targets via the MPC Explorer interface #1139
Conversation
…querying of MPC Explorer API
|
If you would like this to show up in the list of Harvesters by default, you should add it to the |
tom_catalogs/harvesters/mpc.py
Outdated
|
|
||
| target.type = 'NON_SIDEREAL' | ||
| target.scheme = 'MPC_COMET' | ||
| target.name = result['designation_data']['iau_designation'] |
There was a problem hiding this comment.
I think we probably want to handle this more carefully.
I think it might make sense to check the permid, unpacked_primary_provisional_designation, the name, and the unpacked_secondary_provisional_designations.
The issue with the current strategy is that 'iau_designation' results in numbers that are in parentheses (99942) when there is a primary designation and the name isn't captured for named objects.
Other objects can have multiple provisional designations that should also be included as aliases (such as Elyna).
There was a problem hiding this comment.
Note, for reasons I cannot understand, if the object is un-named, "name" isn't in the dictionary at all and will throw a key error instead of just being blank like permid
There was a problem hiding this comment.
This should be addressed now in 2a4b73f
| mock_response.content = [None, 200] | ||
| mock_get.return_value = mock_response | ||
|
|
||
| result = self.broker.query('123456P') |
There was a problem hiding this comment.
self.broker.query doesn't have a return.
This should really be testing that self.catalog_data isn't updated with anything since the return of query will always be None.
There was a problem hiding this comment.
I had trouble with the Mock and the canary actual query making it fail in a useful way and was unsure what the general policy was on incorporating and simulating tests on unreachable or 500 error-returning sites. If you could take a look at potential "predictably failable" queries @jchate6 or point me at other examples (the Simbad Harvester mocks a InconsistentTableError which isn't applicable), I can look at implementing something in addition as I agree that it would be good
I figured that since 1) the MPC Explorer end point doesn't contain comets yet (despite only returning elements in comet form...) and 2) the majority of TOM users would not be using this endpoint, it didn't seem to make a sensible default for the whole project (and I've verified that adding it to FOMO's |
…thesize main name. Add additional tests
There was a problem hiding this comment.
Can you please add instructions on how to include the MPCExplorerHarvester via settings.py in the harvester doc string, and add the Harvester to the list (similar to JPL Horizons harvester) in docs.api.tom_catelog.harvesters.rst?
I'll take care of the form stuff. That might be more complicated.
tom_catalogs/harvesters/mpc.py
Outdated
|
|
||
| # Save Target, adding in additional names | ||
| # target.save(names=extra_desigs) | ||
| target.save() |
There was a problem hiding this comment.
This bypasses the form.
Basically this submits the target during the search phase and populates the form with already saved information.
I understand why this is done like this, but what we actually want is to populate the alias fields in the form and save them after the form is submitted.
I'll work on figuring out how to pipe this through correcty.
There was a problem hiding this comment.
This was the way I could get it work and make the tests pass without getting duplication (in the case of provisional id only) or missing ids. I'm sure it's not the only way though...
There was a problem hiding this comment.
This was much easier than I thought.
There was a problem hiding this comment.
It breaks the tests for me:
======================================================================
ERROR: test_to_target (tom_catalogs.tests.harvesters.test_mpc.TestMPCExplorerHarvester.test_to_target)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/eng/git/tom_base/tom_catalogs/tests/harvesters/test_mpc.py", line 34, in test_to_target
self.assertEqual(target.names, ['65803', 'Didymos', '1996 GT'])
^^^^^^^^^^^^
File "/home/eng/git/tom_base/tom_targets/base_models.py", line 491, in names
return [self.name] + [alias.name for alias in self.aliases.all()]
^^^^^^^^^^^^^^^^^^
File "/home/eng/venv/tombase311_venv/lib64/python3.11/site-packages/django/db/models/manager.py", line 164, in all
return self.get_queryset()
^^^^^^^^^^^^^^^^^^^
File "/home/eng/venv/tombase311_venv/lib64/python3.11/site-packages/django/db/models/fields/related_descriptors.py", line 718, in get_queryset
raise ValueError(
ValueError: 'BaseTarget' instance needs to have a primary key value before this relationship can be used.
The Minor Planet Center's MPC Explorer API interface (used elsewhere in LCO for resolving designations I believe) has recently added support for downloading orbital elements (but sadly not for comets yet... sad snowball...) as well as designations and observations. This PR adds a new
Harvesterwhich supports querying the MPC Explorer to produceTargets.This interface doesn't import the H,G (or the (unpopulated) newer H,G1, G2 or cometary M1, M2, K1, K2) magnitude values but neither does the existing
MPCHarvesterand there's nowhere to put in the baseTargetanyway; possibly this is meant to go intoTargetExtracustom fields?Anyhoo, over to the developers for initial review...