Mpl_normalize refactored and cleaned up by Iquecs2005 · Pull Request #19646 · astropy/astropy

Iquecs2005 · 2026-05-02T20:47:33Z

Description

This pull request is to address the refactoring of mpl_normalize.py, mostly focused on the functions imshow_norm imshow_norm_simple. This pr changes the return of these two functions for a Named Tuple called ImShowNormResults, declared in the same file, containing the AxesImage and ImageNormalize instances. Not only that, but it also separates this function test cases in several other with more asserts.

Fixes #19635

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-05-02T20:47:43Z

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

Thank you !
I think this is going the right direction. Some high level comments for now. In particular I think we should test more than just isinstance calls.

Iquecs2005 · 2026-05-07T01:23:21Z

I think this last commit should address most (if not all) of the feedbacks given.
I added 'TYPE_CHECKING' to the 'matplotlib.image' import, some more assertions to the test, as well as commenting them, and a new "TODO" comment to change 'matplotlib.pyplot' normal import to a lazy one in the future.
Let me know what you guys think!

neutrinoceros

thanks. CI failures are caused by an annotation using a name that is undefined at runtime. I suggested a fix. I recommend paying attention to CI results so you don't need to wait for reviewers to find out that something's broken. I also recommend running the test suite locally before pushing so you get to find out even sooner. Preferably, we wouldn't push broken code into a PR, though it is still useful to do so if you're stuck and you need help deciphering errors.

neutrinoceros · 2026-05-07T09:19:20Z

and a new "TODO" comment to change 'matplotlib.pyplot' normal import to a lazy one in the future.

please revert this. I don't want to set a precedent for this; we have a lot of import statements that could be made lazy in the future, but we already have tooling to discover them automatically so it's no use adding comments for it.

For reference, here's the tooling I'm refering to:
ruff check --select PLC0415 --isolated
https://docs.astral.sh/ruff/rules/import-outside-top-level/

Iquecs2005 · 2026-07-07T18:50:29Z

Hi!
Have you had a chance to look at the changes?
If so, is there something else that I need to do to get this PR aproved?

github-actions · 2026-10-10T05:48:46Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

github-actions · 2026-10-12T05:56:26Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

neutrinoceros · 2026-10-23T09:25:17Z

since we lost the "keep open" label recently, and there wasn't any merge conflict, I took the liberty to rebase your branch to keep the stale bot off for a bit longer, hope you don't mind. I apologize for letting this PR drop at the bottom of my todo list, I'll try to help you get it to the finish line now !

neutrinoceros · 2026-10-23T09:38:00Z

FYI, I'm about to push a couple commits to address my own, previous reviews

neutrinoceros

The only thing left for me to figure out is what, if anything, should I do with the new contributor bot

pllim · 2026-10-23T14:54:28Z

new contributor bot

If you are confident that this PR is good to merge, you can ignore it. That check does not block merge.

pllim · 2026-10-23T14:55:35Z

But I would feel better if visualization maintainers can approve first... @larrybradley or @astrofrog ?

neutrinoceros · 2026-11-02T11:18:09Z

It's been a week without a response. Please @larrybradley or @astrofrog, let me know if you want to have another review or whether this could be merged with just my approval. Thanks !

larrybradley

Thanks for this, @Iquecs2005, and for sticking with it. This PR needs a few cleanups before it can be merged.

larrybradley · 2026-11-06T14:30:41Z

@@ -0,0 +1 @@
+Created ``ImShowNormSimpleResult``, a class inheriting from ``NamedTuple``, and added it as the return type of ``imshow_norm`` and ``imshow_norm_simple``.


The class and function names here don't match the code (ImShowNormResults and imshow_simple_norm). Since tuple unpacking still works, it may also be worth saying that this is backward compatible.

Suggested change

Created ``ImShowNormSimpleResult``, a class inheriting from ``NamedTuple``, and added it as the return type of ``imshow_norm`` and ``imshow_norm_simple``.

``imshow_norm`` and ``imshow_simple_norm`` now return an ``ImShowNormResults`` named tuple with ``image`` and ``norm`` fields. Unpacking the result as a 2-tuple still works.

Also, since nothing breaks, feature might be a better category than api.

larrybradley · 2026-11-06T14:32:13Z

+    image: "AxesImage"
+    """The `~matplotlib.image.AxesImage` instance generated by `~matplotlib.pyplot.imshow`."""
+    norm: ImageNormalize
+    """The `ImageNormalize` instance generated by `SimpleNorm`."""


For imshow_norm, the norm is an ImageNormalize built directly, not one generated by SimpleNorm.

Suggested change

"""The `ImageNormalize` instance generated by `SimpleNorm`."""

"""The `ImageNormalize` instance used as the normalization."""

Also, these attribute docstrings are only picked up by Sphinx. At runtime, ImShowNormResults.norm.__doc__ is just "Alias for field number 1". A numpydoc Attributes section in the class docstring would cover both cases.

larrybradley · 2026-11-06T14:33:34Z

        fig = plt.figure()
        ax = fig.add_subplot(1, 1, 1)
-        im, norm = imshow_norm(image, ax, origin='lower',
+        imNorm = imshow_norm(image, ax, origin='lower',


Small style point: astropy uses snake_case names, and after the rename the continuation lines no longer line up with the opening parenthesis. Something like result = imshow_norm(...) followed by fig.colorbar(result.image) would read more naturally. The same applies to the imshow_simple_norm example below.

larrybradley · 2026-11-06T14:33:54Z

        fig = plt.figure()
        ax = fig.add_subplot(1, 1, 1)
-        im, norm = imshow_simple_norm(image, ax, origin='lower',
+        imNorm = imshow_simple_norm(image, ax, origin='lower',


Same comment as above

larrybradley · 2026-11-06T14:35:06Z

    "scipy-stubs>=1.14.1.6",
-    "narwhals>=1.42.0" # keep in sync with dependency-groups.dataframe
+    "narwhals>=1.42.0", # keep in sync with dependency-groups.dataframe
+    "matplotlib>=3.8.4"


Is this needed? mypy doesn't check astropy.visualization yet, the mypy tox env installs the test extras, and dev already gets matplotlib through astropy[recommended].

larrybradley · 2026-11-06T14:37:10Z

+    imshow_kwargs: Mapping[str | Any] | None = None,
+    simple_norm_imshow_kwargs: Mapping[str | Any] | None = None,


larrybradley · 2026-11-06T14:38:27Z

+    assert_array_equal(
+        ax.imshow(image, **imshow_kwargs).get_array(),
+        results.image.get_array(),
+    )


I don't think this assertion can fail. AxesImage.get_array() returns the input data whatever norm is used. Maybe replace it with a check that the norm was actually applied?

assert results.image.norm is results.norm

larrybradley · 2026-11-06T14:41:05Z

+
+    if simple_norm_imshow_kwargs is None:
+        simple_norm_imshow_kwargs = {}
+    match stretch_spec := simple_norm_imshow_kwargs.get("stretch"):


Nit: the helper is shared by the imshow_norm and imshow_simple_norm tests, so check_imshow_simple_norm_results reads a bit oddly. Also, this match only handles "sinh". An expected_stretch argument checked with isinstance(results.norm.stretch, expected_stretch) would be simpler and work for any stretch.

Iquecs2005 requested review from astrofrog and larrybradley as code owners May 2, 2026 20:47

github-actions Bot added the visualization label May 2, 2026

pllim added this to the v8.1.0 milestone May 4, 2026

pllim reviewed May 4, 2026

View reviewed changes

Comment thread astropy/visualization/mpl_normalize.py Resolved

neutrinoceros reviewed May 5, 2026

View reviewed changes

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

neutrinoceros reviewed May 5, 2026

View reviewed changes

Comment thread astropy/visualization/mpl_normalize.py Resolved

Comment thread astropy/visualization/mpl_normalize.py Resolved

neutrinoceros requested changes May 7, 2026

View reviewed changes

Comment thread astropy/visualization/mpl_normalize.py Resolved

Comment thread astropy/visualization/mpl_normalize.py Resolved

Iquecs2005 requested a review from neutrinoceros May 12, 2026 13:00

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Oct 10, 2026

neutrinoceros removed the Close? Tell stale bot that this issue/PR is stale label Oct 10, 2026

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Oct 11, 2026

neutrinoceros removed the Close? Tell stale bot that this issue/PR is stale label Oct 23, 2026

Iquecs2005 added 6 commits October 23, 2026 11:22

Mpl_normalize Refactored

09fed6d

Type_Checking and Test assertions

4630194

mpl_normalize fix import issues

c355ff1

Create 19646.api.rst

3c7ea9a

Fix docstrings errors

dd1f9d3

Matplotlib tests

6653b95

neutrinoceros force-pushed the mpl_normalize-refactor branch from 8203800 to deba5e8 Compare October 23, 2026 09:22

Iquecs2005 and others added 2 commits October 23, 2026 11:41

Added matplotlib to lowest-resolved-tree.txt

3acc8e8

inline back plt import statements

e147a98

neutrinoceros force-pushed the mpl_normalize-refactor branch from deba5e8 to 78f7411 Compare October 23, 2026 10:59

RFC/TST: unify similar test functions

356c2b3

neutrinoceros force-pushed the mpl_normalize-refactor branch from 78f7411 to 356c2b3 Compare October 23, 2026 13:17

neutrinoceros approved these changes Oct 23, 2026

View reviewed changes

larrybradley requested changes Nov 6, 2026

View reviewed changes

		@@ -0,0 +1 @@
		Created ``ImShowNormSimpleResult``, a class inheriting from ``NamedTuple``, and added it as the return type of ``imshow_norm`` and ``imshow_norm_simple``.

	Created ``ImShowNormSimpleResult``, a class inheriting from ``NamedTuple``, and added it as the return type of ``imshow_norm`` and ``imshow_norm_simple``.
	``imshow_norm`` and ``imshow_simple_norm`` now return an ``ImShowNormResults`` named tuple with ``image`` and ``norm`` fields. Unpacking the result as a 2-tuple still works.

	"""The `ImageNormalize` instance generated by `SimpleNorm`."""
	"""The `ImageNormalize` instance used as the normalization."""

		imshow_kwargs: Mapping[str \| Any] \| None = None,
		simple_norm_imshow_kwargs: Mapping[str \| Any] \| None = None,

Conversation

Iquecs2005 commented May 2, 2026 • edited by neutrinoceros Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented May 2, 2026

Uh oh!

Uh oh!

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Iquecs2005 commented May 7, 2026

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented May 7, 2026

Uh oh!

Iquecs2005 commented Jul 7, 2026

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

neutrinoceros commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Oct 23, 2026

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 23, 2026

Uh oh!

pllim commented Oct 23, 2026

Uh oh!

neutrinoceros commented Nov 2, 2026

Uh oh!

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

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!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Iquecs2005 commented May 2, 2026 •

edited by neutrinoceros

Loading

neutrinoceros commented Oct 23, 2026 •

edited

Loading