Deprecate raw response from non-async method #3089

mkelley · 2024-09-02T13:39:10Z

Instead, direct users to use the async method.

A test was updated to use the async method for the raw response.
Cleaned up a few keyword parameters that actually didn't do anything, including some get_raw_response keywords.
Actually use the cache parameter in get_ephemerides.
Renamed a method to indicate that it is private (it isn't really useful outside of this module).
Updated remote tests. In particular, moved uncertainty column testing to use mocked data to avoid replacing yet another target that does not return uncertainty info anymore.

2024 AA does not return uncertainty info anymore. Rather than continuously chase objects with uncertianties, just test mocked data.

keflavich · 2024-09-02T17:02:50Z

Thanks! This change brings mpc more in line with the standard astroquery interface. 👍 from me.

bsipocz

Some minor comments, mostly to do the deprecations properly (at least for one version cycle), and to clarify a bit around the test changes.

And while you're doing generic cleanup and maintenance, I wonder if you would mind having a look at the failing doctests in the narrative documentation, too. (pytest docs/mpc -R should do the trick of running them)

bsipocz · 2024-09-04T00:54:46Z

astroquery/mpc/core.py

@@ -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):


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.

bsipocz · 2024-09-04T00:56:15Z

astroquery/mpc/core.py

@@ -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):


changelog says deprecate, yet outright removed. Consider adding the decorator for now.

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.

The same comment for get_observatory_codes.

Mentioned these changes (and justification) in the change log.

bsipocz · 2024-09-04T00:57:12Z

astroquery/mpc/core.py

@@ -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",


This is what needs to be done for the other methods, too.

Suggested change

@deprecated_renamed_argument("get_raw_response", None, since="0.4.9",

@deprecated_renamed_argument("get_raw_response", None, since="0.4.8",

bsipocz · 2024-09-04T00:57:26Z

astroquery/mpc/core.py

    def get_observations_async(self, targetid, *,
                               id_type=None,
-                               comettype=None,


what happened with this parameter?

It was an unused parameter and doesn't correspond to anything in the API. I can make a comment in the change log.

Mentioned in the change log.

bsipocz · 2024-09-04T01:00:28Z

astroquery/mpc/tests/test_mpc_remote.py

@@ -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):


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.

~~The uncertainty column is effectively tested with the test_get_ephemeris_unc_links method. Perhaps the method should be renamed.~~

This is the remote file, and these are definitely the tests that need to be removed.

bsipocz · 2024-09-04T01:00:44Z

astroquery/mpc/tests/test_mpc.py

@@ -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):


what are the reason for removing these tests?

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.

I partially thought it was a mistake because test_get_ephemeris_Moon_phase_and_Uncertainty was defined twice.

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 reverts commit 289408c.

mkelley · 2024-09-06T00:25:37Z

I've edited the doctest, too. It should now pass.

bsipocz

Thanks @mkelley!

mkelley added 6 commits September 2, 2024 08:24

Remove unused get_raw_response parameter.

819ca22

Remove unused comettype parameter.

f7a3737

Rename undocumented private method.

906917c

Respsect cache parameter.

f1c2791

Deprecate get_raw_response

cd91eae

Use a comet that does not fail; Use mocked data for uncertainty tests.

c56a4fe

2024 AA does not return uncertainty info anymore. Rather than continuously chase objects with uncertianties, just test mocked data.

mkelley added the mpc label Sep 2, 2024

mkelley added 3 commits September 2, 2024 09:41

Remove unused import.

087b0f0

Remove misplaced code.

289408c

Changelog entry

783d7e3

mkelley added 2 commits September 2, 2024 14:59

Fix method name in docstring.

c3ac42a

Also comment on fixed bug.

bd1079a

bsipocz added this to the v0.4.8 milestone Sep 4, 2024

bsipocz reviewed Sep 4, 2024

View reviewed changes

mkelley added 5 commits September 5, 2024 19:55

Revert "Remove misplaced code."

88e103e

This reverts commit 289408c.

Remove redundant 1994 XG based tests in favor of 2024 AA.

258f945

Fix deprecation: since 0.4.8

f29553a

More thoroughly identify changes.

e6b7b8d

Use 2P/Encke for doctest since Hyakutake isn't currently available.

274984f

bsipocz approved these changes Sep 6, 2024

View reviewed changes

bsipocz merged commit e8fff0b into astropy:main Sep 6, 2024
9 checks passed

mkelley deleted the mpc-deprecate-raw-response branch September 6, 2024 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate raw response from non-async method #3089

Deprecate raw response from non-async method #3089

mkelley commented Sep 2, 2024

keflavich commented Sep 2, 2024

bsipocz left a comment

bsipocz Sep 4, 2024

mkelley Sep 6, 2024

bsipocz Sep 4, 2024

mkelley Sep 5, 2024 •

edited

Loading

mkelley Sep 6, 2024

mkelley Sep 6, 2024

bsipocz Sep 4, 2024

mkelley Sep 6, 2024

bsipocz Sep 4, 2024

mkelley Sep 5, 2024

mkelley Sep 6, 2024

bsipocz Sep 4, 2024

mkelley Sep 5, 2024 •

edited

Loading

mkelley Sep 6, 2024

bsipocz Sep 4, 2024

mkelley Sep 5, 2024

mkelley Sep 6, 2024 •

edited

Loading

mkelley commented Sep 6, 2024

bsipocz left a comment

	@deprecated_renamed_argument("get_raw_response", None, since="0.4.9",
	@deprecated_renamed_argument("get_raw_response", None, since="0.4.8",

		@@ -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):

Deprecate raw response from non-async method #3089

Deprecate raw response from non-async method #3089

Conversation

mkelley commented Sep 2, 2024

keflavich commented Sep 2, 2024

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

Choose a reason for hiding this comment

mkelley Sep 5, 2024 • edited Loading

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

mkelley Sep 5, 2024 • edited Loading

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

mkelley Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

mkelley commented Sep 6, 2024

bsipocz left a comment

Choose a reason for hiding this comment

mkelley Sep 5, 2024 •

edited

Loading

mkelley Sep 5, 2024 •

edited

Loading

mkelley Sep 6, 2024 •

edited

Loading