Update MPC query to parse 3 digit precision motion columns #3026
Conversation
|
cc @mkelley |
|
Thank you for identifying and addressing these issues, @jurezakrajsek . Let me know if you want help debugging the failures. I'm investigating the first two and see that the columns are not yet fully aligned. See the last two columns from the test_get_ephemeris_Moon_phase_and_Uncertainty() test: It is the same problem in the test_get_ephemeris_Uncertainty() test. |
|
@mkelley |
|
Found a better example for the test: |
…recision Modified test and data set for 3 digit sky motion precision
|
@mkelley I have added a new data set of the object 2024 AA which covers this case of 3 digit sky motion and also has the uncertainty. I also added a test for an empty ephemeris table that I encountered last week for comet 340P, but I can no longer repeate this issue (looks like something strange was on the MPC side), so I manually created the test data file by removing the ephemeris lines from the result table. |
There was a problem hiding this comment.
I would prefer to see remote-data tests added, too, not just mocked ones with a canned HTML response.
There was a problem hiding this comment.
For empty ephemeris result I was not able to repeate the error for comet 340P.
There was a time period that MPC services had some issues, so Im not sure if it was related to that but it happened, so it does make sence to capture this
Therefor it is difficult to create a remote test for this case as I cannot find a target and date where such a response would be returned.
Anyway the remote tests for ephemeris are just testing if we get the response back.
@pytest.mark.parametrize('target', ['(3202)', 'C/2003 A2'])
def test_get_ephemeris_by_target(self, target):
# test that query succeeded
response = mpc.MPC.get_ephemeris(target)
assert len(response) > 0
Should we include the tests for Moon_phase and Moon_phase_and_Uncertainty like in these in test_mpc.py also?
def test_get_ephemeris_Moon_phase(patch_post):
result = mpc.core.MPC.get_ephemeris('2P', location='G37')
assert result['Moon phase'][0] >= 0
def test_get_ephemeris_Uncertainty(patch_post):
# this test requires an object with uncertainties != N/A
result = mpc.core.MPC.get_ephemeris('2024 AA')
assert result['Uncertainty 3sig'].quantity[0] > 0 * u.arcsec
There was a problem hiding this comment.
yes, that would be great, at the minimum I suppose you can just add these objects to the previous target list above and just test if results are returned and parsed out.
But it also good to add the other two extra tests that do some check for the content.
(In general we try to minimize the testing load on the remote services, but these are quite fast queries without downloading hundred of Mb of data, so we should indeed test against the server more).
There was a problem hiding this comment.
Tests for ephemeris parsing are now added to the test_mpc_remote.py
Running
pytest -P mpc -m remote_data --remote-data=any --cov astroquery/mpc --cov-config=setup.cfg
runs with 21 passed tests.
astroquery\mpc\tests\test_mpc_remote.py ..................... [100%]
---------- coverage: platform win32, python 3.10.10-final-0 ----------
Name Stmts Miss Cover
-----------------------------------------------------
astroquery\mpc\__init__.py 11 0 100%
astroquery\mpc\core.py 382 111 71%
astroquery\mpc\setup_package.py 4 2 50%
-----------------------------------------------------
TOTAL 397 113 72%
=== 21 passed, 60 deselected in 32.70s ===
Also running a local test
pytest -P mpc --cov astroquery/mpc --cov-config=setup.cfg
runs with 60 passed tests.
astroquery\mpc\tests\test_mpc.py ........................................................... [ 72%]
astroquery\mpc\tests\test_mpc_remote.py sssssssssssssssssssss [ 98%]
docs\mpc\mpc.rst . [100%]
---------- coverage: platform win32, python 3.10.10-final-0 ----------
Name Stmts Miss Cover
-----------------------------------------------------
astroquery\mpc\__init__.py 11 0 100%
astroquery\mpc\core.py 382 44 88%
astroquery\mpc\setup_package.py 4 2 50%
-----------------------------------------------------
TOTAL 397 46 88%
=== 60 passed, 21 skipped in 4.32s ===
|
Unrelated, but as I was thinking about the empty responses and when it would make sense to get an Error vs a Warning, I checked the known impactors and was surprised to see emphemeris returned for them. Isn't that something to be fixed at server side? |
… table is retrieved - Added remote test for ephemeris with moon phase and uncertainty - Changes listed in CHANGES.rst
|
@bsipocz Example, if you check periodic comets: On the other hand, Difference between P/2014 U2 and P/2011 WG113 And to make it even more confusing, the orbital elements are not the same to its official 365P There are many of these first-apparition designations that have an official number; some return an ephemeris response, others don't. |
|
The MPC is working to replace all of their previous services with modern and documented API end points, including this ephemeris service, which should improve its behavior and the relationships between temporary and permanent designations. |
astroquery/mpc/core.py
Outdated
| # raise EmptyResponseError if no ephemeris lines are found in the query response | ||
| try: | ||
| i = text_table.index('\n', text_table.index('JD_TT')) + 1 | ||
| except ValueError as e: |
There was a problem hiding this comment.
Please add a test that covers this exception.
There was a problem hiding this comment.
@mkelley
It is difficult to create a remote test for this case as I can no longer repeate this issue by querying the MPC for the same object and date that i described in the issu (looks like something was not working on the MPC side at that time). I'm not able to find a different target or date where such a response would be returned. But if it happens I can add a remote test for this case also.
So I have manually created the test data file by removing the ephemeris lines from the result table and included a test for this in local data test_mpc.py
def test_get_ephemeris_by_name_empty(patch_post):
with pytest.raises(NoResultsWarning):
mpc.core.MPC.get_ephemeris('340P', location='G37')
There was a problem hiding this comment.
I think the manual test is OK. I can't think of a successful remote test case that should return empty lines. Brigitta's impactor example is an interesting idea, but the service clearly doesn't behave that way.
There was a problem hiding this comment.
I can't think of a successful remote test case that should return empty line
well, that case I would revert my previous opinion about the warning and say that it should loudly error as an indication of something is broken.
There was a problem hiding this comment.
I agree, EmptyResponseError
There was a problem hiding this comment.
@mkelley
I have corrected it back to EmptyResponseerror
astroquery/mpc/core.py
Outdated
| # raise EmptyResponseError if no ephemeris lines are found in the query response | ||
| try: | ||
| i = text_table.index('\n', text_table.index('h m s')) + 1 | ||
| except ValueError as e: |
There was a problem hiding this comment.
e is not used, so probably shouldn't be defined.
astroquery/mpc/core.py
Outdated
| # raise EmptyResponseError if no ephemeris lines are found in the query response | ||
| try: | ||
| i = text_table.index('\n', text_table.index('JD_TT')) + 1 | ||
| except ValueError as e: |
There was a problem hiding this comment.
Thanks. Finally, e is not used, so probably shouldn't be defined.
astroquery/mpc/tests/test_mpc.py
Outdated
| @@ -64,7 +64,7 @@ | |||
| from astropy.coordinates import EarthLocation, Angle | |||
| from astropy.time import Time | |||
|
|
|||
| from ...exceptions import InvalidQueryError | |||
| from ...exceptions import EmptyResponseError, InvalidQueryError, NoResultsWarning | |||
There was a problem hiding this comment.
Remove the unused NoResultsWarning
CHANGES.rst
Outdated
| @@ -12,6 +12,8 @@ mpc | |||
| ^^^ | |||
|
|
|||
| - Parse star catalog information when querying observations database [#2957] | |||
| - Parse ephemeris with sky motion with three digit precision [#3019] | |||
| - Raise NoResultWarining when empty ephemeris reponse is returned [#3019] | |||
There was a problem hiding this comment.
NoResultWarning -> EmptyResponseError
There was a problem hiding this comment.
Also, instead of referencing the issues, reference the PR #3026.
|
@mkelley |
#3019 update of the MPC parse_results to correctly parse 3 digit precision motion columns
#3022 update of the MPC parse_results to raise EmptyResponseError if no table lines are parsed