renamed and deprecated Observations and Observations class for the gemini and mast module by tinuademargaret · Pull Request #1885 · astropy/astroquery

tinuademargaret · 2020-11-04T06:26:36Z

No description provided.

codecov · 2020-11-04T06:30:26Z

Codecov Report

Merging #1885 (e68e664) into main (96f63eb) will increase coverage by 1.56%.
The diff coverage is 100.00%.

❗ Current head e68e664 differs from pull request most recent head 65cbaad. Consider uploading reports for the commit 65cbaad to get more accurate results

@@            Coverage Diff             @@
##             main    #1885      +/-   ##
==========================================
+ Coverage   62.87%   64.44%   +1.56%     
==========================================
  Files         133      200      +67     
  Lines       17246    15961    -1285     
==========================================
- Hits        10844    10286     -558     
+ Misses       6402     5675     -727

Impacted Files	Coverage Δ
astroquery/gemini/__init__.py	`100.00% <100.00%> (ø)`
astroquery/gemini/core.py	`69.56% <100.00%> (-1.13%)`	⬇️
astroquery/mast/__init__.py	`100.00% <100.00%> (ø)`
astroquery/mast/observations.py	`75.52% <100.00%> (-0.90%)`	⬇️
astroquery/heasarc/core.py	`21.48% <0.00%> (-52.31%)`	⬇️
astroquery/utils/system_tools.py	`52.17% <0.00%> (-31.16%)`	⬇️
astroquery/mast/core.py	`78.26% <0.00%> (-12.37%)`	⬇️
astroquery/gaia/core.py	`61.31% <0.00%> (-10.63%)`	⬇️
astroquery/esa/hubble/core.py	`76.10% <0.00%> (-10.03%)`	⬇️
astroquery/utils/url_helpers.py	`85.00% <0.00%> (-9.12%)`	⬇️
... and 162 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

ceb8

This is a massive breaking change for users, we need to have a deprecation period where the old syntax still works but a deprecation message is displayed. Check out https://docs.astropy.org/en/stable/api/astropy.utils.decorators.deprecated.html#astropy.utils.decorators.deprecated

ceb8 · 2020-11-11T18:15:26Z

CHANGES.rst

 Service fixes and enhancements
 ------------------------------

+observations


There should be two changelog entries, one under MAST and one under Gemeni.

This is a massive breaking change for users, we need to have a deprecation period where the old syntax still works but a deprecation message is displayed. Check out https://docs.astropy.org/en/stable/api/astropy.utils.decorators.deprecated.html#astropy.utils.decorators.deprecated

Would it be a good idea to assign the old name of the class to the new name
e.g ObservationsClass = GeminiObservationsClass()
and display a deprecated message saying ObservationsClass is deprecated. Use GeminiObservationsClass instead

You can use this PR as an example for renaming and deprecating a class: astropy/astropy#9445

Nice! Thanks @bsipocz

ceb8 · 2020-11-11T18:18:10Z

@olyoberdorf You probably want to take a look at this too.

olyoberdorf · 2020-11-11T18:40:43Z

Gemini is very new, but the deprecation work would be essentially identical to what's needed for MAST. So you may as well do both. Also, thanks for working on my module :)

bsipocz

Overall this looks good to me, and I only have some minor comments, one of them needs input from @ceb8.

However, the documentation should also be updated before we merge this.

bsipocz · 2020-11-14T22:40:54Z

astroquery/gemini/core.py

        "dec"]

+
+@deprecated(since='v0.4.1', alternative='GeminiObservationsClass')


we're in 0.4.2 dev cycle, so this should say deprecated since v0.4.2

bsipocz · 2020-11-14T22:44:49Z

CHANGES.rst

+gemini
+^^^^^^
+
+- Renamed and deprecated Observations and ObservationsClass in GEMINI module. [#1885]


this is nitpicking, so I only point out to show better practice: changelog should point out what has changed, and what the new solution is. No need to mention the module name in the changelog as the section above already contains that information. The backticks are to highlight the class names when rendered for the documentation.

Suggested change

- Renamed and deprecated Observations and ObservationsClass in GEMINI module. [#1885]

- Renamed and deprecated ``Observations`` and ``ObservationsClass`` to ``GeminiObservations`` and ``GeminiObservationsClass`` . [#1885]

bsipocz · 2020-11-14T22:45:42Z

CHANGES.rst

+mast
+^^^^
+
+- Renamed and deprecated Observations and ObservationsClass in MAST module. [#1885]


similarly

Suggested change

- Renamed and deprecated Observations and ObservationsClass in MAST module. [#1885]

- Renamed and deprecated ``Observations`` and ``ObservationsClass`` to ``MastObservations`` and ``MastObservationsClass``. [#1885]

bsipocz · 2020-11-14T22:47:06Z

astroquery/gemini/__init__.py

+from .core import GeminiObservationsClass, GeminiObservations, Observations, ObservationsClass

-__all__ = ['Observations', 'ObservationsClass', 'conf']
+__all__ = ['Observations', 'ObservationsClass', 'GeminiObservations', 'GeminiObservationsClass', 'conf']


I think we don't recommend wildchar imports, so removing the old class names from the namespace is probably safe.

bsipocz · 2020-11-14T22:57:21Z

Oh, and as I see there are some conflicts due to a PR that has been merged in the meantime, so this will need a rebase at some point (better to do it once you've done the docs updates, too).

Also, note to maintainers that travis is gone, and I have to fix the CI before we can reasonably merge anything.

ceb8 · 2020-11-16T17:22:18Z

@bsipocz Which of your comments did you need my input on?

Also thanks for remembering the documentation! Super important!

bsipocz · 2021-04-20T01:12:51Z

This needs a rebase due to conflicts. I may try to get to it before release to make sure the deprecations end up in the release.

pep8speaks · 2021-07-01T17:36:52Z

Hello @tinumide! 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-05-05 08:04:42 UTC

bsipocz

Some tiny comments. Fixing these and the pep8 issues should make this ready to go I think

bsipocz · 2021-07-01T21:26:47Z

astroquery/gemini/core.py



-__all__ = ['Observations', 'ObservationsClass']  # specifies what to import
+<<<<<<< HEAD


there are some remnants here from the conflict resolutions

bsipocz · 2021-07-01T21:27:17Z

astroquery/gemini/core.py

        "dec"]

+
+@deprecated(since='v0.4.2', alternative='GeminiObservationsClass')


this is now 0.4.3

bsipocz · 2021-07-01T21:27:30Z

astroquery/mast/observations.py

        return self._portal_api_connection.service_request_async(service, params, pagesize, page, **kwargs)


+@deprecated(since='v0.4.2', alternative='MastObservationsClass')


bsipocz · 2021-07-02T03:38:56Z

~~Good news, I see the docs build error on this branch locally, too. Bad news, I don't see the reason, the same build passes.~~ See inline comment for the fix.

The deprecation of the instances in a way that the warning is not raised at module import time is I suppose the last outstanding issue, but there I don't yet have a solution.

bsipocz · 2021-07-02T03:39:01Z

And I spotted one more issue, if you do an import the deprecation warnings should not show. I only have an ugly workaround for the class instance and I'm not sure how I could force it to emit a deprecation when used, but not when the module is imported.

Python 3.9.1 (default, Jan 11 2021, 15:50:22) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.20.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import astroquery.mast
WARNING: AstropyDeprecationWarning: The ObservationsClass class is deprecated and may be removed in a future version.
        Use MastObservationsClass instead. [astroquery.mast.observations]

Maybe @pllim you have a nice idea to work this around?

bsipocz · 2021-07-02T03:57:06Z

astroquery/mast/__init__.py


 conf = Conf()

-from .cutouts import TesscutClass, Tesscut, ZcutClass, Zcut


Keep this line unchanged, that will fix the docs build.

pllim · 2021-07-02T14:21:15Z

astroquery/gemini/core.py

+
+
+GeminiObservations = GeminiObservationsClass()
 Observations = ObservationsClass()


This line is causing the warning. I guess the brute force way is to silence the warning just for this line...?

Yes, the issue is that if I put a filter in __init__.py, then we rightly don't get the warning at module import time. However, then we don't get it at all for any usage of Observations, and that's what probably users used the most as the docs is full of Observations.xzy().

bsipocz · 2021-07-03T00:43:03Z

I'm sorry, but this has to wait for the next release, I just can't find a way to have all our cases covered, to make sure all users who use the old names to see the warning, but those using the new one don't.

bsipocz · 2022-03-18T04:23:23Z

Heads up @ceb8 @jaymedina @tomdonaldson @olyoberdorf: I would like to go ahead with this sooner rather than later. Would you be still on board? Basically we already followed the same logic with the new class in mast, calling it e.g. MastMissions.

It is understood that we'll need long deprecation period, for both modules so for a while all the users will experience is a deprecation warning at the time of import.

As I see the last comments in here, this got stalled with some issues around getting the right warning in all situations. Besides fixing that, do you see any blockers with the proposed idea?

olyoberdorf · 2022-03-18T04:44:54Z

Hi Brigitta, Yeah still on board. I don’t recall any other problems. I’m on vacation but I am back this weekend.

…

On Fri, Mar 18, 2022 at 12:23 PM Brigitta Sipőcz ***@***.***> wrote: Heads up @ceb8 <https://github.com/ceb8> @jaymedina <https://github.com/jaymedina> @tomdonaldson <https://github.com/tomdonaldson> @olyoberdorf <https://github.com/olyoberdorf>: I would like to go ahead with this sooner rather than later. Would you be still on board? Basically we already followed the same logic with the new class in mast, calling it e.g. MastMissions. It is understood that we'll need long deprecation period, for both modules so for a while all the users will experience is a deprecation warning at the time of import. As I see the last comments in here, this got stalled with some issues around getting the right warning in all situations. Besides fixing that, do you see any blockers with the proposed idea? — Reply to this email directly, view it on GitHub <#1885 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACKOSDNTXBHFATOFQT4PA2TVAQAMNANCNFSM4TJUHZZA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

renamed and deprecated Observations and Observations class in mast module added changelog entry gemini observations old syntax now works gemini observations old syntax still works added old syntax to init some corrections mast observations old syntax still works changes requested updated gemini doc updated mast doc renamed and deprecated Observations and Observations class fixed pep8 issues and corrected deprecated version added Zcut renamed obesrvation in test files renamed and deprecated Observations and Observations class

astropy-bot bot added the gemini label Nov 4, 2020

tinuademargaret changed the title ~~renamed and deprecated Observations and Observations class for the gemini module~~ renamed and deprecated Observations and Observations class for the gemini and mast module Nov 4, 2020

bsipocz added the mast label Nov 4, 2020

bsipocz added this to the v0.4.2 milestone Nov 4, 2020

ceb8 requested changes Nov 11, 2020

View reviewed changes

bsipocz requested changes Nov 14, 2020

View reviewed changes

tinuademargaret force-pushed the issue-1748 branch from 96f228a to 9f45736 Compare November 21, 2020 12:51

tinuademargaret requested review from bsipocz and ceb8 November 21, 2020 12:54

bsipocz modified the milestones: v0.4.2, v0.4.3 May 15, 2021

tinuademargaret force-pushed the issue-1748 branch from 9f45736 to f3c1630 Compare July 1, 2021 17:36

bsipocz reviewed Jul 1, 2021

View reviewed changes

bsipocz reviewed Jul 2, 2021

View reviewed changes

tinuademargaret force-pushed the issue-1748 branch from 71e377d to ecfc7fd Compare July 2, 2021 09:28

pllim reviewed Jul 2, 2021

View reviewed changes

bsipocz modified the milestones: v0.4.3, v0.4.4 Jul 3, 2021

bsipocz modified the milestones: v0.4.4, v0.4.5 Nov 12, 2021

bsipocz modified the milestones: v0.4.5, v4 Dec 25, 2021

bsipocz force-pushed the issue-1748 branch from ecfc7fd to 65cbaad Compare May 5, 2022 08:04

		"dec"]


		@deprecated(since='v0.4.1', alternative='GeminiObservationsClass')

	- Renamed and deprecated Observations and ObservationsClass in GEMINI module. [#1885]
	- Renamed and deprecated ``Observations`` and ``ObservationsClass`` to ``GeminiObservations`` and ``GeminiObservationsClass`` . [#1885]



		__all__ = ['Observations', 'ObservationsClass'] # specifies what to import
		<<<<<<< HEAD

		"dec"]


		@deprecated(since='v0.4.2', alternative='GeminiObservationsClass')

		return self._portal_api_connection.service_request_async(service, params, pagesize, page, **kwargs)


		@deprecated(since='v0.4.2', alternative='MastObservationsClass')


		conf = Conf()

		from .cutouts import TesscutClass, Tesscut, ZcutClass, Zcut



		GeminiObservations = GeminiObservationsClass()
		Observations = ObservationsClass()

Uh oh!

Conversation

tinuademargaret commented Nov 4, 2020

Uh oh!

codecov bot commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ceb8 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ceb8 commented Nov 11, 2020

Uh oh!

olyoberdorf commented Nov 11, 2020

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Nov 14, 2020

Uh oh!

ceb8 commented Nov 16, 2020

Uh oh!

bsipocz commented Apr 20, 2021

Uh oh!

pep8speaks commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2022-05-05 08:04:42 UTC

Uh oh!

bsipocz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Jul 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Jul 3, 2021

Uh oh!

bsipocz commented Mar 18, 2022

Uh oh!

olyoberdorf commented Mar 18, 2022 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Nov 4, 2020 •

edited

Loading

pep8speaks commented Jul 1, 2021 •

edited

Loading

bsipocz left a comment •

edited

Loading

bsipocz commented Jul 2, 2021 •

edited

Loading