-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Deprecate raw response from non-async method #3089
Changes from 11 commits
819ca22
f7a3737
906917c
f1c2791
cd91eae
c56a4fe
087b0f0
289408c
783d7e3
c3ac42a
bd1079a
88e103e
258f945
f29553a
e6b7b8d
274984f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,6 +17,7 @@ | |||||
from . import conf | ||||||
from ..utils import async_to_sync, class_or_instance | ||||||
from ..exceptions import InvalidQueryError, EmptyResponseError | ||||||
from astropy.utils.decorators import deprecated_renamed_argument | ||||||
|
||||||
__all__ = ['MPCClass'] | ||||||
|
||||||
|
@@ -186,7 +187,7 @@ def query_object_async(self, target_type, *, get_query_payload=False, **kwargs): | |||||
|
||||||
""" | ||||||
|
||||||
self.get_mpc_object_endpoint(target_type) | ||||||
self._get_mpc_object_endpoint(target_type) | ||||||
|
||||||
kwargs['limit'] = 1 | ||||||
return self.query_objects_async(target_type, get_query_payload=get_query_payload, **kwargs) | ||||||
|
@@ -315,7 +316,7 @@ def query_objects_async(self, target_type, *, get_query_payload=False, **kwargs) | |||||
Limit the number of results to the given value | ||||||
|
||||||
""" | ||||||
mpc_endpoint = self.get_mpc_object_endpoint(target_type) | ||||||
mpc_endpoint = self._get_mpc_object_endpoint(target_type) | ||||||
|
||||||
if (target_type == 'comet'): | ||||||
kwargs['order_by_desc'] = "epoch" | ||||||
|
@@ -329,7 +330,7 @@ def query_objects_async(self, target_type, *, get_query_payload=False, **kwargs) | |||||
auth = (self.MPC_USERNAME, self.MPC_PASSWORD) | ||||||
return self._request('GET', mpc_endpoint, params=request_args, auth=auth) | ||||||
|
||||||
def get_mpc_object_endpoint(self, target_type): | ||||||
def _get_mpc_object_endpoint(self, target_type): | ||||||
mpc_endpoint = self.MPC_URL | ||||||
if target_type == 'asteroid': | ||||||
mpc_endpoint = mpc_endpoint + '/search_orbits' | ||||||
|
@@ -344,8 +345,7 @@ def get_ephemeris_async(self, target, *, location='500', start=None, step='1d', | |||||
proper_motion='total', proper_motion_unit='arcsec/h', | ||||||
suppress_daytime=False, suppress_set=False, | ||||||
perturbed=True, unc_links=False, | ||||||
get_query_payload=False, | ||||||
get_raw_response=False, cache=False): | ||||||
get_query_payload=False, cache=False): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changelog says deprecate, yet outright removed. Consider adding the decorator for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one was removed rather than deprecated because the parameter was unused and had no effect on the method's behavior. I can deprecate it even though it doesn't do anything, or just add a comment in the change log. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same comment for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentioned these changes (and justification) in the change log. |
||||||
r""" | ||||||
Object ephemerides from the Minor Planet Ephemeris Service. | ||||||
|
||||||
|
@@ -436,10 +436,6 @@ def get_ephemeris_async(self, target, *, location='500', start=None, step='1d', | |||||
Return the HTTP request parameters as a dictionary | ||||||
(default: ``False``). | ||||||
|
||||||
get_raw_response : bool, optional | ||||||
Return raw data without parsing into a table (default: | ||||||
``False``). | ||||||
|
||||||
cache : bool | ||||||
Defaults to False. If set overrides global caching behavior. | ||||||
See :ref:`caching documentation <astroquery_cache>`. | ||||||
|
@@ -486,7 +482,7 @@ def get_ephemeris_async(self, target, *, location='500', start=None, step='1d', | |||||
| P/2003 CP7 | Comet P/2003 CP7 (LINEAR-NEAT) | | ||||||
+------------+-----------------------------------+ | ||||||
|
||||||
For comets, P/ and C/ are interchangable. The designation | ||||||
For comets, P/ and C/ are interchangeable. The designation | ||||||
may also be in a packed format: | ||||||
|
||||||
+------------+-----------------------------------+ | ||||||
|
@@ -601,21 +597,18 @@ def get_ephemeris_async(self, target, *, location='500', start=None, step='1d', | |||||
return request_args | ||||||
|
||||||
self.query_type = 'ephemeris' | ||||||
response = self._request('POST', self.MPES_URL, data=request_args) | ||||||
response = self._request('POST', self.MPES_URL, data=request_args, cache=cache) | ||||||
|
||||||
return response | ||||||
|
||||||
@class_or_instance | ||||||
def get_observatory_codes_async(self, *, get_raw_response=False, cache=True): | ||||||
def get_observatory_codes_async(self, *, cache=True): | ||||||
""" | ||||||
Table of observatory codes from the IAU Minor Planet Center. | ||||||
|
||||||
|
||||||
Parameters | ||||||
---------- | ||||||
get_raw_response : bool, optional | ||||||
Return raw data without parsing into a table (default: | ||||||
`False`). | ||||||
|
||||||
cache : bool | ||||||
Defaults to True. If set overrides global caching behavior. | ||||||
|
@@ -771,9 +764,10 @@ def _args_to_ephemeris_payload(self, **kwargs): | |||||
return request_args | ||||||
|
||||||
@class_or_instance | ||||||
@deprecated_renamed_argument("get_raw_response", None, since="0.4.9", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what needs to be done for the other methods, too.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||||||
alternative="async methods") | ||||||
def get_observations_async(self, targetid, *, | ||||||
id_type=None, | ||||||
comettype=None, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happened with this parameter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was an unused parameter and doesn't correspond to anything in the API. I can make a comment in the change log. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentioned in the change log. |
||||||
get_mpcformat=False, | ||||||
get_raw_response=False, | ||||||
get_query_payload=False, | ||||||
|
@@ -783,6 +777,11 @@ def get_observations_async(self, targetid, *, | |||||
from the `Minor Planet Center observations database | ||||||
<https://minorplanetcenter.net/db_search>`_. | ||||||
|
||||||
.. deprecated:: 0.4.9 | ||||||
The ``get_raw_response`` keyword argument is deprecated. The | ||||||
`~MPCClass.get_observations_async` method will return a raw response. | ||||||
|
||||||
|
||||||
Parameters | ||||||
---------- | ||||||
targetid : int or str | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,7 +156,7 @@ def test_args_to_object_payload(): | |
('asteroid', | ||
'https://minorplanetcenter.net/web_service/search_orbits')]) | ||
def test_get_mpc_object_endpoint(type, url): | ||
query_url = mpc.core.MPC.get_mpc_object_endpoint(target_type=type) | ||
query_url = mpc.core.MPC._get_mpc_object_endpoint(target_type=type) | ||
assert query_url == url | ||
|
||
|
||
|
@@ -176,19 +176,6 @@ def test_get_ephemeris_Moon_phase(patch_post): | |
assert result['Moon phase'][0] >= 0 | ||
|
||
|
||
def test_get_ephemeris_Uncertainty(patch_post): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are the reason for removing these tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this is a mistake. I thought I had made a copy-paste error, but these were actually working mocked tests. I can restore them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I partially thought it was a mistake because I think PR #3026 attempted to replace 1994 XG with 2024 AA, even though in the mocked tests it wasn't needed (because the mocked data didn't change, only the online results). I'll clean this up. |
||
# 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 | ||
|
||
|
||
def test_get_ephemeris_Moon_phase_and_Uncertainty(patch_post): | ||
# this test requires an object with uncertainties != N/A | ||
result = mpc.core.MPC.get_ephemeris('2024 AA', location='G37') | ||
assert result['Moon phase'][0] >= 0 | ||
assert result['Uncertainty 3sig'].quantity[0] > 0 * u.arcsec | ||
|
||
|
||
def test_get_ephemeris_by_name_empty(patch_post): | ||
with pytest.raises(EmptyResponseError): | ||
mpc.core.MPC.get_ephemeris('340P', location='G37') | ||
|
@@ -383,9 +370,16 @@ def test_get_ephemeris_perturbed(perturbed, val): | |
|
||
@pytest.mark.parametrize('unc_links', (True, False)) | ||
def test_get_ephemeris_unc_links(unc_links, patch_post): | ||
tab = mpc.core.MPC.get_ephemeris('1994 XG', unc_links=unc_links) | ||
assert ('Unc. map' in tab.colnames) == unc_links | ||
assert ('Unc. offsets' in tab.colnames) == unc_links | ||
result = mpc.core.MPC.get_ephemeris('1994 XG', unc_links=unc_links) | ||
assert np.all(result['Uncertainty 3sig'].quantity > 0 * u.arcsec) | ||
assert ('Unc. map' in result.colnames) == unc_links | ||
assert ('Unc. offsets' in result.colnames) == unc_links | ||
|
||
|
||
def test_get_ephemeris_Moon_phase_and_Uncertainty(patch_post): | ||
result = mpc.core.MPC.get_ephemeris('1994 XG', location='G37') | ||
assert np.all(result['Moon phase'][0] >= 0) | ||
assert np.all(result['Uncertainty 3sig'].quantity > 0 * u.arcsec) | ||
|
||
|
||
def test_get_observatory_codes(patch_get): | ||
|
@@ -415,10 +409,8 @@ def test_get_observations(patch_get): | |
assert result['DEC'].unit == u.deg | ||
assert result['epoch'].unit == u.d | ||
|
||
result = mpc.core.MPC.get_observations('12893', | ||
get_raw_response=True) | ||
|
||
assert result[0]['designation'] == "1998 QS55" | ||
result = mpc.core.MPC.get_observations_async('12893') | ||
assert result.json()[0]['designation'] == "1998 QS55" | ||
|
||
result = mpc.core.MPC.get_observations('12893', | ||
get_mpcformat=True) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
# Licensed under a 3-clause BSD style license - see LICENSE.rst | ||
import requests | ||
import pytest | ||
import astropy.units as u | ||
from astroquery.exceptions import InvalidQueryError | ||
from astroquery import mpc | ||
|
||
|
@@ -72,7 +71,7 @@ def test_get_observatory_codes(self): | |
greenwich = ['000', 0.0, 0.62411, 0.77873, 'Greenwich'] | ||
assert all([r == g for r, g in zip(response[0], greenwich)]) | ||
|
||
@pytest.mark.parametrize('target', ['(3202)', 'C/2003 A2']) | ||
@pytest.mark.parametrize('target', ['(3202)', 'C/2024 N4']) | ||
def test_get_ephemeris_by_target(self, target): | ||
# test that query succeeded | ||
response = mpc.MPC.get_ephemeris(target) | ||
|
@@ -82,17 +81,6 @@ def test_get_ephemeris_Moon_phase(self): | |
result = mpc.core.MPC.get_ephemeris('2P', location='G37') | ||
assert result['Moon phase'][0] >= 0 | ||
|
||
def test_get_ephemeris_Uncertainty(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are the reason for removing these? I see that you mention to cleanup some tests to only run the mock versions but these seem to be removed for the mock test, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the remote file, and these are definitely the tests that need to be removed. |
||
# this test requires an object with uncertainties != N/A | ||
result = mpc.core.MPC.get_ephemeris('2024 AA', start='2024-06-15') | ||
assert result['Uncertainty 3sig'].quantity[0] > 0 * u.arcsec | ||
|
||
def test_get_ephemeris_Moon_phase_and_Uncertainty(self): | ||
# this test requires an object with uncertainties != N/A | ||
result = mpc.core.MPC.get_ephemeris('2024 AA', location='G37', start='2024-06-15') | ||
assert result['Moon phase'][0] >= 0 | ||
assert result['Uncertainty 3sig'].quantity[0] > 0 * u.arcsec | ||
|
||
def test_get_ephemeris_target_fail(self): | ||
# test that query failed | ||
with pytest.raises(InvalidQueryError): | ||
|
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.
this is technically an API change (though wasn't used in the docs, nor has any docstrings in the API docs), so I would say, mentioning the cleanup the changelog would be sufficient.
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.
Done.