API: reintroduce private classes from `astropy.visualization.lupton_rgb` that were removed without deprecation in astropy 7.0.0 by neutrinoceros · Pull Request #17531 · astropy/astropy

neutrinoceros · 2024-12-11T14:48:29Z

Description

Follow up to #15081
Fixes #17528

I implemented the backward compatibility layer in a form that easily allows deprecation: re-introduced classes are isolated in their own (explicitly private) module, and re-exposed where needed, leaving room for an actual deprecation message.
However I didn't add the deprecation warning yet: this is meant to be backported, and my understanding is that deprecations cannot be added in patch releases. Maybe a pending deprecation is warranted ? in any case, we can properly deprecate these classes in astropy 7.1, with the recommended alternative being "just copy paste the source code if you really need these classes" (or maybe something more appropriate, @sedonaprice ?) ?

Note that I didn't find any doc or tests specifically attached to these classes and that should be ported from astropy 6.1.0

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-12-11T14:48:59Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2024-12-11T14:49:02Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

neutrinoceros · 2024-12-11T14:54:14Z

+were dropped without deprecation in astropy 7.0.0, were re-introduced following
+a report that they were used downstream.
+``Mapping``, ``AsinhMapping``, ``LinearMapping``, ``AsinhZScaleMapping``
+use at your own risks


This is a placeholder sentence that I'd like to revise with the following information once we've converged upon reviews:

are these classes tested and/or documented (probably not) ?

are they actually deprecated, pending deprecation, or none of the above ?

If you check out v6.1.x branch and grep for the names, it would probably tell you about test/doc situation. The API doc might be found (or not) at https://docs.astropy.org/en/v6.1.x/ .

Looks like we can go ahead with deprecation according to comments from @astrofrog and @timj .

Actually I went the other way and declared them as public, following @saimn (and my) sentiment that there is no reason to disrupt users without a proper alternative.

pllim · 2024-12-11T18:11:59Z

Re: Deprecation -- If we want to be pedantic, it has to be "pending" until 8.0.0 now. But given that they were completely removed in 7.0.0, showing back up in 7.0.1 but deprecated (for real) is still a step forward, so it is up for discussion... 🤷‍♀️

Maybe @saimn can chime in here as release manager for 7.0.0.

sedonaprice · 2024-12-11T18:12:16Z

@neutrinoceros ---

(1) Re recommended alternatives:
One option is certainly to copy the source code of the v6.1 and prior classes, reintroduced in the private module.
Another alternative would be to suggest class(es) that inherits from the refactored class RGBImageMappingLupton as described here: #15081 (comment) (shorter amount of total code to copy; preserves the initialization and method AsinhMapping.make_rgb_image(image_r, image_g, image_b) as used in the prior class.)

(Happy to write an example implementation of a AsinhZScaleMapping class that inherits from RGBImageMappingLupton if that's helpful, and to write testing methods that demonstrate these modified inheriting backwards-compatibility classes fulfill the tests of the v6.1 classes [see below].)

(2) Re testing and documentation for Mapping, AsinhMapping, LinearMapping, AsinhZScaleMapping:
There were tests for AsinhMapping, AsinhZScaleMapping, LinearMapping, in v6.1.x, see lines L122 - L171, L205 - L220:

astropy/astropy/visualization/tests/test_lupton_rgb.py

Lines 122 to 171 in 5692198

    
               def test_Asinh(self): 
        
                   """Test creating an RGB image using an asinh stretch""" 
        
                   asinhMap = lupton_rgb.AsinhMapping(self.min_, self.stretch_, self.Q) 
        
                   rgbImage = asinhMap.make_rgb_image(self.image_r, self.image_g, self.image_b) 
        
                   if display: 
        
                       display_rgb(rgbImage, title=sys._getframe().f_code.co_name) 
        
               def test_AsinhZscale(self): 
        
                   """Test creating an RGB image using an asinh stretch estimated using zscale""" 
        
                   map = lupton_rgb.AsinhZScaleMapping(self.image_r, self.image_g, self.image_b) 
        
                   rgbImage = map.make_rgb_image(self.image_r, self.image_g, self.image_b) 
        
                   if display: 
        
                       display_rgb(rgbImage, title=sys._getframe().f_code.co_name) 
        
               def test_AsinhZscaleIntensity(self): 
        
                   """ 
        
                   Test creating an RGB image using an asinh stretch estimated using zscale on the intensity 
        
                   """ 
        
                   map = lupton_rgb.AsinhZScaleMapping(self.image_r, self.image_g, self.image_b) 
        
                   rgbImage = map.make_rgb_image(self.image_r, self.image_g, self.image_b) 
        
                   if display: 
        
                       display_rgb(rgbImage, title=sys._getframe().f_code.co_name) 
        
               def test_AsinhZscaleIntensityPedestal(self): 
        
                   """Test creating an RGB image using an asinh stretch estimated using zscale on the intensity 
        
                   where the images each have a pedestal added""" 
        
                   pedestal = [100, 400, -400] 
        
                   self.image_r += pedestal[0] 
        
                   self.image_g += pedestal[1] 
        
                   self.image_b += pedestal[2] 
        
                   map = lupton_rgb.AsinhZScaleMapping( 
        
                       self.image_r, self.image_g, self.image_b, pedestal=pedestal 
        
                   ) 
        
                   rgbImage = map.make_rgb_image(self.image_r, self.image_g, self.image_b) 
        
                   if display: 
        
                       display_rgb(rgbImage, title=sys._getframe().f_code.co_name) 
        
               def test_AsinhZscaleIntensityBW(self): 
        
                   """Test creating a black-and-white image using an asinh stretch estimated 
        
                   using zscale on the intensity""" 
        
                   map = lupton_rgb.AsinhZScaleMapping(self.image_r) 
        
                   rgbImage = map.make_rgb_image(self.image_r, self.image_r, self.image_r) 
        
                   if display: 
        
                       display_rgb(rgbImage, title=sys._getframe().f_code.co_name)

astropy/astropy/visualization/tests/test_lupton_rgb.py

Lines 205 to 220 in 5692198

    
           def test_linear(self): 
        
               """Test using a specified linear stretch""" 
        
               map = lupton_rgb.LinearMapping(-8.45, 13.44) 
        
               rgbImage = map.make_rgb_image(self.image_r, self.image_g, self.image_b) 
        
               if display: 
        
                   display_rgb(rgbImage, title=sys._getframe().f_code.co_name) 
        
           def test_linear_min_max(self): 
        
               """Test using a min/max linear stretch determined from one image""" 
        
               map = lupton_rgb.LinearMapping(image=self.image_b) 
        
               rgbImage = map.make_rgb_image(self.image_r, self.image_g, self.image_b) 
        
               if display: 
        
                   display_rgb(rgbImage, title=sys._getframe().f_code.co_name)

--- these test methods would mostly need to renamed if added to test_lupton_rgb.py, or could be introduced as a separate file test_lupton_rgb_depr_private_classes.py
with tests directly copied from v6.1 (https://github.com/astropy/astropy/blob/v6.1.x/astropy/visualization/tests/test_lupton_rgb.py)
(The only documentation for these methods I saw was in the methods' docstrings.)

astrofrog · 2024-12-11T22:08:24Z

Re: deprecations - these classes should have been deprecated in 7.0.0 so I think in this case it is acceptable to add the deprecation in 7.0.1.

saimn · 2024-12-12T07:59:55Z

Is there an alternative to these classes ? If they provided a feature that is used and has no clear alternative, why deprecate those ?

neutrinoceros · 2025-01-02T14:16:03Z

If we opt to not deprecated them, maybe we should at least expose them as clearly public somewhere.

timj · 2025-01-07T19:52:56Z

Quick comment that Rubin is using AsinhMapping in multiple places (it was code we contributed to astropy). We would have preferred they not be deprecated at all but giving us a chance to work out how to work around the removal would be much appreciated.

pllim · 2025-01-07T20:07:31Z

Since deprecation will be disruptive for a whole observatory, should we include pending=True (i.e., no visible warning) for now and just make the deprecation period longer? Does not hurt to keep it for a little while, right?

neutrinoceros · 2025-01-08T10:23:59Z

If we do not offer an alternative then deprecation (pending or not) would only delay the point of disruption, wouldn't it ?

Rubin is using AsinhMapping in multiple places

@timj would it be acceptable for you to move these classes to an isolated package and add it as a dependency where needed ? this seems like the only way to go if we're deprecating. If that is not practical, I'm thinking we should probably embrace these classes as public instead of deprecating them ?

mfisherlevine · 2025-01-08T16:04:29Z

For whatever it's worth (and I don't really know the full context here), I (as a member of the Rubin team) would love them not to disappear at all - asinh and asinh+zscale are both useful to us in several places.

pllim · 2025-01-09T16:18:30Z

@astrofrog , can we keep both forever?

timj · 2025-01-09T16:21:35Z

We have been discussing this internally in Rubin. We realize that we should have been more explicit about what we felt was public API vs internal API. For now we are pinning to <6 but we think that if you merge this PR and have the classes available with deprecation message until v8 we can change our code to work around the removal.

pllim · 2025-01-09T23:05:02Z

Maybe add a __all__ too?

I actually moved the classes back to where they lived in astropy 6.1 and added them to that module's __all__

astrofrog · 2025-01-10T10:21:15Z

Re: deprecation - if there isn't an obvious simple replacement then I say we keep the classes and don't deprecate them, since they did look somewhat public before. We can always re-explore this in future to see if we can simplify things and if we want to deprecate anything, but unless there is a good reason to, we should keep them.

pllim · 2025-01-23T16:51:52Z

The problem from cosmology is confusing.

astropy/cosmology/_src/core.py:docstring of astropy.cosmology._src.core.FlatCosmologyMixin.clone:1: WARNING: more than one target found for cross-reference 'Mapping': astropy.modeling.mappings.Mapping, astropy.visualization.Mapping, astropy.visualization.lupton_rgb.Mapping [ref.python]

It is either

astropy/astropy/cosmology/_src/core.py

Line 562 in a1ebee5

self, *, meta: Mapping | None = None, to_nonflat: bool = False, **kwargs

or

astropy/astropy/cosmology/_src/core.py

Line 571 in a1ebee5

meta : mapping or None (optional, keyword-only)

@nstarman any idea? You using any ref magicks?

pllim · 2025-01-23T16:53:57Z

p.s. Why are we reusing "mapping" so much?

astrofrog · 2025-01-23T17:02:08Z

I've not had a chance to look into this but why was this not an issue before?

pllim · 2025-01-23T17:20:53Z

This dup name thingy looks so familiar but I don't remember how we patched it before. Maybe it was patched for cosmo vs modeling, but now that viz introduces another one, it broke again.

pllim · 2025-01-23T17:29:40Z

What if it is sphinx-doc/sphinx#10400 this time? Should we disable type checking in RTD? Is it enabled?

timj · 2025-01-23T17:32:26Z

The Mapping class was there in astropy 6 but the mapping classes weren't lifted into the parent namespace. I suggested up above that we restrict what the parent __init__.py lifts into the astropy.visualization namespace by doing something like from .lupton_rgb import make_lupton_rgb instead of import *.

nstarman · 2025-01-23T18:59:11Z

Ref: #10130 and astropy/astropy-APEs#85 for solution.
The refactor of cosmology, which aligns with astropy/astropy-APEs#85, ensures that no private API leakage can occur from cosmology (except for Sphinx-related bugs re typing (see also https://github.com/sphinx-extensions2/sphinx-autodoc2 for static doc builders that don't suffer from dynamic-related bugs)).

astrofrog · 2025-01-23T21:19:49Z

@neutrinoceros - could we try what @timj suggested here? #17531 (comment)

neutrinoceros · 2025-01-27T09:57:26Z

Done. Sorry it took me a couple days to get back to this !

pllim · 2025-01-27T20:38:44Z

RTD passes now. @astrofrog , are you able to do a final review? Thanks! 🙏

astrofrog · 2025-01-28T00:08:15Z

On my list for tomorrow!

astrofrog

This seems good to go once the small issue with the changelog formatting mentioned by @pllim above is fixed

…b that were removed without deprecation in astropy 7.0.0

neutrinoceros · 2025-01-29T08:02:34Z

Thanks, I somehow missed it ! done !

lumberbot-app · 2025-01-30T19:39:46Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v7.0.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 d847a5c1715faadcd2be40741f1682a6838e1006

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #17531: API: reintroduce private classes from `astropy.visualization.lupton_rgb` that were removed without deprecation in astropy 7.0.0'

Push to a named branch:

git push YOURFORK v7.0.x:auto-backport-of-pr-17531-on-v7.0.x

Create a PR against branch v7.0.x, I would have named this PR:

"Backport PR #17531 on branch v7.0.x (API: reintroduce private classes from astropy.visualization.lupton_rgb that were removed without deprecation in astropy 7.0.0)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

pllim · 2025-01-30T19:40:17Z

Oh no, auto backport failed. @neutrinoceros , do you mind manually backporting this? Thanks!

neutrinoceros · 2025-01-30T20:59:40Z

Sure. I'll handle it tomorrow!

…e classes from `astropy.visualization.lupton_rgb` that were removed without deprecation in astropy 7.0.0) API: reintroduce private classes from `astropy.visualization.lupton_rgb` that were removed without deprecation in astropy 7.0.0

neutrinoceros · 2025-01-31T07:46:11Z

done in #17697

Backport PR #17531 on branch `v7.0.x` (API: reintroduce private classes from `astropy.visualization.lupton_rgb` that were removed without deprecation in astropy 7.0.0)

neutrinoceros added this to the v7.0.1 milestone Dec 11, 2024

neutrinoceros added Bug API change PRs and issues that change an existing API, possibly requiring a deprecation period labels Dec 11, 2024

github-actions Bot added the visualization label Dec 11, 2024

neutrinoceros force-pushed the visualization/api/17528/reintroduce_mapping_classes branch from 25f4f4d to 3a5a2b8 Compare December 11, 2024 14:51

neutrinoceros mentioned this pull request Dec 11, 2024

"from astropy.visualization.lupton_rgb import AsinhMapping" broken in v7.0.0 without deprecation #17528

Closed

neutrinoceros commented Dec 11, 2024

View reviewed changes

neutrinoceros marked this pull request as ready for review December 11, 2024 14:54

neutrinoceros requested review from astrofrog and larrybradley as code owners December 11, 2024 14:54

pllim added the backport-v7.0.x label Dec 11, 2024

timj mentioned this pull request Jan 7, 2025

DM-48321: Ignore build files lsst/scarlet_lite#9

Merged

2 tasks

pllim reviewed Jan 9, 2025

View reviewed changes

neutrinoceros force-pushed the visualization/api/17528/reintroduce_mapping_classes branch 2 times, most recently from 66ecc27 to 416e8e9 Compare January 10, 2025 09:04

neutrinoceros force-pushed the visualization/api/17528/reintroduce_mapping_classes branch 2 times, most recently from d5946dc to 99b5842 Compare January 27, 2025 09:56

neutrinoceros marked this pull request as ready for review January 27, 2025 09:57

pllim reviewed Jan 27, 2025

View reviewed changes

Comment thread docs/changes/visualization/17531.bugfix.rst

astrofrog approved these changes Jan 28, 2025

View reviewed changes

API: reintroduce private classes from astropy.visualization.lupton_rg…

3708ed7

…b that were removed without deprecation in astropy 7.0.0

neutrinoceros force-pushed the visualization/api/17528/reintroduce_mapping_classes branch from d848c04 to 3708ed7 Compare January 29, 2025 08:02

pllim merged commit d847a5c into astropy:main Jan 30, 2025

lumberbot-app Bot added the Still Needs Manual Backport label Jan 30, 2025

neutrinoceros deleted the visualization/api/17528/reintroduce_mapping_classes branch January 30, 2025 20:59

neutrinoceros mentioned this pull request Jan 31, 2025

Backport PR #17531 on branch v7.0.x (API: reintroduce private classes from astropy.visualization.lupton_rgb that were removed without deprecation in astropy 7.0.0) #17697

Merged

1 task

neutrinoceros removed the Still Needs Manual Backport label Jan 31, 2025

Uh oh!

Conversation

neutrinoceros commented Dec 11, 2024

Description

Uh oh!

github-actions Bot commented Dec 11, 2024

Uh oh!

github-actions Bot commented Dec 11, 2024

Uh oh!

neutrinoceros Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

pllim Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

pllim commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedonaprice commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Dec 11, 2024

Uh oh!

saimn commented Dec 12, 2024

Uh oh!

neutrinoceros commented Jan 2, 2025

Uh oh!

timj commented Jan 7, 2025

Uh oh!

pllim commented Jan 7, 2025

Uh oh!

neutrinoceros commented Jan 8, 2025

Uh oh!

mfisherlevine commented Jan 8, 2025

Uh oh!

pllim commented Jan 9, 2025

Uh oh!

timj commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Jan 10, 2025

Uh oh!

pllim commented Jan 23, 2025

Uh oh!

pllim commented Jan 23, 2025

Uh oh!

astrofrog commented Jan 23, 2025

Uh oh!

pllim commented Jan 23, 2025

Uh oh!

pllim commented Jan 23, 2025

Uh oh!

timj commented Jan 23, 2025

Uh oh!

nstarman commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Jan 23, 2025

Uh oh!

neutrinoceros commented Jan 27, 2025

Uh oh!

Uh oh!

pllim commented Jan 27, 2025

Uh oh!

astrofrog commented Jan 28, 2025

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Jan 29, 2025

Uh oh!

pllim commented Dec 11, 2024 •

edited

Loading

sedonaprice commented Dec 11, 2024 •

edited

Loading

timj commented Jan 9, 2025 •

edited

Loading

nstarman commented Jan 23, 2025 •

edited

Loading