Fix / test mpc error messages 2022.07 #2466

mkelley · 2022-07-13T15:20:56Z

Update the MPC.get_observations error message to direct the user to the Minor Planet Center in order to help determine if the error is due to a problem with astroquery or MPC.

Also, test MPC.get_ephemeris with an object that exists, but apparently has no orbital elements in the database.

Addresses #2464

pep8speaks · 2022-07-13T15:20:59Z

Hello @mkelley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-13 15:21:54 UTC

codecov · 2022-07-13T15:33:36Z

Codecov Report

Merging #2466 (1ec3b88) into main (7277870) will decrease coverage by 2.94%.
The diff coverage is n/a.

❗ Current head 1ec3b88 differs from pull request most recent head 6097b39. Consider uploading reports for the commit 6097b39 to get more accurate results

@@            Coverage Diff             @@
##             main    #2466      +/-   ##
==========================================
- Coverage   65.86%   62.92%   -2.94%     
==========================================
  Files         233      133     -100     
  Lines       17907    17302     -605     
==========================================
- Hits        11795    10888     -907     
- Misses       6112     6414     +302

Impacted Files	Coverage Δ
astroquery/mpc/core.py	`88.32% <ø> (-0.07%)`	⬇️

... and 180 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

bsipocz · 2022-07-13T16:04:45Z

astroquery/mpc/core.py

+                                    'identifiers correct?  Is the MPC '
+                                    'database search working for your '
+                                    'object? The service is hosted at '
+                                    'https://www.minorplanetcenter.net/'


I actually liked the previous error message more, and at minimum we should use the URL from a variable rather than hard wire in here. Maybe the overall issue should be rather mentioned in the docs? (E.g. as everywhere in astroqeury, if a query doesn't work with the web interface then it's not really an issue for astroquery but either an upstream issue/bug or user error)

What if the error instead simply reported what it found? EmptyResponseError("The query returned no observations.")

I think there are two main cases when there are no results. One, that should raise an error is when e.g. the identifiers are incorrect, and the service may even return an error itself. OTOH I can think of valid empty responses when all inputs are correct, just there are simply no observations in the given region/epoch for the given object.

Here we have the first case, I suppose, and therefore an EmptyResponseError is a good approach (or a ValueError if we also identify the issue with one of the input parameters, but I don't think we are checking on those)

eerovaher · 2022-07-13T16:50:13Z

astroquery/mpc/core.py

@@ -1184,7 +1184,11 @@ def _parse_result(self, result, **kwargs):

            if len(src) == 0:
                raise RuntimeError(('No data queried. Are the target '


Not directly related to the issue, but astroquery defines a more appropriate error type:

astroquery/astroquery/exceptions.py

Line 109 in f2d0bd2

class EmptyResponseError(ValueError):

yes, there are certainly better error types than the RunTimeError, e.g. in these case even a ValueError may be more appropriate.

I agree, RuntimeError is not the right one. EmptyResponseError: Astroquery error class to be raised when the query returns an empty result. For this error the result is an empty list, i.e., []. If that is OK, then I'd like to use EmptyResponseError.

yes, I think that's appropriate (my reading is that EmptyResponseError should be raised when query results are expected but empty is what's returned (as opposed to legit empty response when e.g. there are no matches with the query parameters, but because there are no observations of the given region etc., that case e.g. an empty table can be returned)

Are we saying, then, that we want to have EmptyResponseError instead of RuntimeError before merging this PR? (I'm asking mostly to bump this PR, which otherwise looks useful)

It would be good to change the error type, but because these lines of code were not introduced in this pull request then I don't think this should be a requirement for merging.

I went ahead and did the change, we could merge when CI is happy.

bsipocz

I'm OK with this PR as is, but left a few comments regarding the error type we raise.

I think I would not rephrase the error message, I find it verbose and informative enough (it points out there may be some issues with the input rather than no observations), but if you think the extended message would better direct users to check their inputs on the mpc services when running into issues, I'm OK for having the extended message.

bsipocz · 2022-07-26T17:09:26Z

astroquery/mpc/core.py

@@ -1184,7 +1184,11 @@ def _parse_result(self, result, **kwargs):

            if len(src) == 0:
                raise RuntimeError(('No data queried. Are the target '


yes, I think that's appropriate (my reading is that EmptyResponseError should be raised when query results are expected but empty is what's returned (as opposed to legit empty response when e.g. there are no matches with the query parameters, but because there are no observations of the given region etc., that case e.g. an empty table can be returned)

bsipocz · 2022-07-26T17:13:34Z

astroquery/mpc/core.py

+                                    'identifiers correct?  Is the MPC '
+                                    'database search working for your '
+                                    'object? The service is hosted at '
+                                    'https://www.minorplanetcenter.net/'


I think there are two main cases when there are no results. One, that should raise an error is when e.g. the identifiers are incorrect, and the service may even return an error itself. OTOH I can think of valid empty responses when all inputs are correct, just there are simply no observations in the given region/epoch for the given object.

Here we have the first case, I suppose, and therefore an EmptyResponseError is a good approach (or a ValueError if we also identify the issue with one of the input parameters, but I don't think we are checking on those)

There is a relevant online test failure (should be easy to fix), so I revoke this approval for now

An object without observations suggests a problem at the MPC.

… empty responses

mkelley · 2023-05-09T13:37:52Z

Thanks!

bsipocz reviewed Jul 13, 2022

View reviewed changes

eerovaher reviewed Jul 13, 2022

View reviewed changes

bsipocz added the mpc label Jul 26, 2022

bsipocz previously approved these changes Jul 26, 2022

View reviewed changes

bsipocz added this to the v0.4.7 milestone Sep 11, 2022

bsipocz mentioned this pull request Sep 16, 2022

BUG: MPC no results for some named asteroids #2531

Closed

mkelley and others added 5 commits May 9, 2023 09:27

Test MPC ephemeris service failure for valid object without orbit.

ad2f110

Update MPC error message for observations lookup without data.

342a727

An object without observations suggests a problem at the MPC.

Remove whitespace in blank line

1674875

MAINT: switching RuntimeError to be an EmptyResponseError instead for…

fecf1b3

… empty responses

MAINT: cleanup legacy import

6097b39

bsipocz force-pushed the fix-mpc-error-messages-2022.07 branch from 1ec3b88 to 6097b39 Compare May 9, 2023 07:39

bsipocz approved these changes May 9, 2023

View reviewed changes

bsipocz merged commit f39fcf8 into astropy:main May 9, 2023
7 checks passed

mkelley deleted the fix-mpc-error-messages-2022.07 branch May 9, 2023 13:37

mkelley mentioned this pull request Jun 5, 2023

Observations not found for numerous objects which are in the MPC database #2464

Closed

bsipocz added the no-changelog-entry-needed label Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix / test mpc error messages 2022.07 #2466

Fix / test mpc error messages 2022.07 #2466

mkelley commented Jul 13, 2022

pep8speaks commented Jul 13, 2022 •

edited

codecov bot commented Jul 13, 2022 •

edited

bsipocz Jul 13, 2022

mkelley Jul 22, 2022

bsipocz Jul 26, 2022

eerovaher Jul 13, 2022

bsipocz Jul 13, 2022

mkelley Jul 22, 2022

bsipocz Jul 26, 2022

keflavich May 5, 2023

eerovaher May 8, 2023

bsipocz May 9, 2023

bsipocz left a comment

bsipocz Jul 26, 2022

bsipocz Jul 26, 2022

mkelley commented May 9, 2023

		@@ -1184,7 +1184,11 @@ def _parse_result(self, result, **kwargs):

		if len(src) == 0:
		raise RuntimeError(('No data queried. Are the target '

Fix / test mpc error messages 2022.07 #2466

Fix / test mpc error messages 2022.07 #2466

Conversation

mkelley commented Jul 13, 2022

pep8speaks commented Jul 13, 2022 • edited

Comment last updated at 2022-07-13 15:21:54 UTC

codecov bot commented Jul 13, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkelley commented May 9, 2023

pep8speaks commented Jul 13, 2022 •

edited

codecov bot commented Jul 13, 2022 •

edited