add imshow_simple_norm by eteq · Pull Request #19623 · astropy/astropy

eteq · 2026-04-30T04:51:07Z

Description

This PR adds a function @larrybradley and I discussed in the hazy distant past when imshow_norm was first made: a convenient version of it that uses simple_norm's interface instead of ImageNormalize directly. The gist is that this allows a convenient and compact specification of imshow like this:

imshow_norm(image, min_percent=1, max_percent=99.9, stretch='log')

which makes imports more compact for simple quick-viz cases.

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 · 2026-04-30T04:51:24Z

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.

neutrinoceros

Hi Erik ! welcome to the astropy project, we hope you'll enjoy the contribution process !

neutrinoceros · 2026-04-30T05:36:27Z

+        fig.colorbar(im)
+    """
+    if ax is None:
+        if not HAS_MATPLOTLIB:


why not move the import dance a the top level of the function ?

This was just following from the above - but I think the motivation is that it's possible (albiet unlikely) that the user might pass in an ax object that is not from matplotlib but some matplotlib look-alike. And in that case we might as well try instead of erroring out. But if they give None we have to have plt to do plt.gca()

So I'd rather not change it since it is addressing a conceivable (albiet unlikely) use case.

we might as well try instead of erroring out

I think this hits close to a point I fundamentally disagree with. As a user, I much prefer my tools to say "sorry, I don't know what to do with this", than getting some uncontrolled error message nobody actually thought about later on.
But, you have status quo on your side, and there's no point making this function acting different than the rest of the API, so I won't push for it. Just wanted to highlight this one philosophical point of friction.

(anecdoctally, I think we, as a community, effectively trained users to see any traceback as a question mark (is this a bug, or it is just not supported ?), to the point I've seen people reporting NotImplementedErrors, asking if they found a bug, or if they could somehow work around it)

I'm not sure I agree with your broader point, but I'm not sure I disagree - that is, it's definitely a good point I'm just not sure how to balance the two conflicting desires/concerns. That said, I think, as you pointed out, this is a problem shared with the other function, so I'd rather we discuss this in a separate follow-on PR/issue (ideally after feature freeze! Since its been there for several years I think there's little harm in waiting another 6 mo to fix/change this)

oh I can definitely get behind this plan ! I don't mean to block this, I'm just treating it as an opportunity to discover what hidden (to me) corners of astropy have in store

neutrinoceros · 2026-04-30T05:37:08Z

+            "want that."
+        )
+
+    imshow_kwargs = dict(kwargs)


better reflects what I think is the intention here

Suggested change

imshow_kwargs = dict(kwargs)

imshow_kwargs = kwargs.copy()

neutrinoceros · 2026-04-30T05:40:13Z

+            "want that."
+        )
+
+    imshow_kwargs = dict(kwargs)


actually why even bother with a copy ? this dict is owned by the function, we can mutate it as needed

It's because the next few lines split up the kwargs between snorm_kwargs and imshow_kwargs, and I felt it was confusing to leave one with just the name kwargs. And once I'm doing that it seemed better to keep kwargs itself pristine, and to me the dict(kwargs) construct better reflects the intention of "creating a new dictionary with a different purpose, but starting out with the same content as kwargs until we start popping things from it a few lines below".

That said, I guess the following would be equivalent as long as no one changes this in the future to use kwargs below:

Suggested change

imshow_kwargs = dict(kwargs)

imshow_kwargs = kwargs

and is like a few microseconds more performant 🤷 . I'm fine with it either way so whoever ends up merging I'm comfortable with them either applying the change above or not as they like.

neutrinoceros · 2026-04-30T05:46:58Z

+    imshow_kwargs["norm"] = SimpleNorm(**snorm_kwargs)(data)
+    imshow_result = ax.imshow(data, **imshow_kwargs)
+
+    return imshow_result, imshow_kwargs["norm"]


returning a dataclass instance would make it instantly clearer how to access which part. And if you absolutely need or want the tuple interface (mostly, unpacking ?), you could use a NamedTuple instead.

I disagree. I think it is more important that this function is consistent with imshow_norm above (an existing implementation not added by this PR).

Agree with @pllim !

making return values into NamedTuple would be backward compatible and can easily be made consistently throughout the API. Though, if it's a consistency issue, better to stick with what you have for this PR. What do you guys think of moving to NamedTuples later ?

NamedTuple should be discussed as a new issue. Ultimately, as long as the changes here are consistent with existing implementation, I think it should be acceptable unless it is a bug. Thanks, all!

neutrinoceros · 2026-04-30T05:48:49Z

    return imshow_result, imshow_kwargs["norm"]
+
+
+def imshow_simple_norm(data, ax=None, **kwargs):


none of the arguments are easy to annotate, but the return value is, and that's very useful in itself

Unfortunately, this is not as clean as one would like because I would need to import matplotlib to include the full annotation which triggers all the optional dependency annoyances. But I did add just tuple which is better than nothing!

for reference, this is how you do it for a type-only import

from typing import TYPE_CHECKING if TYPE_CHECKING: from matplotlib import MyType def foo() -> "MyType": ...

the reason this works is that typing.TYPE_CHECKING is exactly False at runtime, but the standard specifies that type checkers treat it as True.

neutrinoceros · 2026-04-30T05:49:42Z



+@pytest.mark.skipif(not HAS_PLT, reason="requires matplotlib")
+def test_imshow_simple_norm():


looks like 3 image tests in a trench coat to me

I think it is fine as long as it passes?

Yeah, I was just following the other test like this - I think at the time the sub-package maintainers (which was I think @larrybradley and someone else?) preferred it that way for the convenience functions. I'm not against changing that, but I'd prefer to defer it to later when it can be done for the rest of the tests in the sub-pagcakge.

My issue is that, in their current form, I can't really say that these test actually check the output. Without changing the entire nature of the test, adding a couple more assertions would already help.

@eteq To get this in before feature freeze, could you add a few more assertions?

Point taken, done in the last commit! Probably would make sense to port those tests back to simple_imshow but I think that's not this PR so as not to mix cocnerns.

Co-authored-by: P. L. Lim <[email protected]> Co-authored-by: Erik Tollerud <[email protected]>

eteq · 2026-05-01T16:36:10Z

I believe the last couple of commits addressed all the comments (@neutrinoceros may not entirely agree but I think we're on the same page of "not critical so could be addressed in a PR after feature freeze to let this get in today)

larrybradley

Thanks, @eteq. We can address the followup issues after feature freeze.

pllim · 2026-05-01T17:46:23Z

I opened #19635 as follow-up. Hopefully I didn't miss anything. Feel free to correct if I did.

Thanks, all!

eteq added 2 commits April 29, 2026 23:53

add imshow_simple_norm

00e2aa7

fix inappropriate use of simplenorm

748ab05

eteq requested review from astrofrog and larrybradley as code owners April 30, 2026 04:51

github-actions Bot added the visualization label Apr 30, 2026

eteq added 2 commits April 30, 2026 00:55

add changelog entry

2de7206

double-backticks to fix docs error

b29d9f4

neutrinoceros reviewed Apr 30, 2026

View reviewed changes

pllim reviewed Apr 30, 2026

View reviewed changes

Comment thread docs/changes/visualization/19623.feature.rst Resolved

pllim reviewed Apr 30, 2026

View reviewed changes

Comment thread astropy/visualization/tests/test_norm.py Resolved

pllim reviewed Apr 30, 2026

View reviewed changes

Comment thread astropy/visualization/mpl_normalize.py Resolved

Apply suggestions from @pllim and @neutrinoceros

1a37309

Co-authored-by: P. L. Lim <[email protected]> Co-authored-by: Erik Tollerud <[email protected]>

pllim added this to the v8.0.0 milestone May 1, 2026

eteq added 2 commits May 1, 2026 12:32

add return type annotation

6dad1f2

add some output image asserts

eba5381

larrybradley approved these changes May 1, 2026

View reviewed changes

pllim merged commit b3807dd into astropy:main May 1, 2026
37 of 38 checks passed

pllim mentioned this pull request May 1, 2026

RFC: Clean up mpl_normalize implementation #19635

Open

		return imshow_result, imshow_kwargs["norm"]


		def imshow_simple_norm(data, ax=None, **kwargs):



		@pytest.mark.skipif(not HAS_PLT, reason="requires matplotlib")
		def test_imshow_simple_norm():

Conversation

eteq commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Apr 30, 2026

Uh oh!

neutrinoceros 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!

neutrinoceros May 1, 2026 • 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!

eteq May 1, 2026 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Apr 30, 2026 • 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!

neutrinoceros May 1, 2026 • 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!

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!

Uh oh!

Uh oh!

Uh oh!

eteq commented May 1, 2026

Uh oh!

larrybradley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pllim commented May 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

eteq commented Apr 30, 2026 •

edited

Loading

neutrinoceros May 1, 2026 •

edited

Loading

eteq May 1, 2026 •

edited

Loading

neutrinoceros Apr 30, 2026 •

edited

Loading

neutrinoceros May 1, 2026 •

edited

Loading