Check if the mappable is in a different Figure than the one fig.color… #27458

zoehcycy · 2023-12-07T05:37:45Z

…bar is being called on.

Ignore this check if the mappable has no host Figure. Warn in case of a mismatch.

PR summary

PR checklist

"closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
Plotting related features are demonstrated in an example
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

…bar is being called on. Ignore this check if the mappable has no host Figure. Warn in case of a mismatch.

zoehcycy · 2023-12-07T05:42:48Z

Hi! This is my first time contributing to matplotlib.
This commit resolves:
[Bug]: Colorbar cannot be added to another figure
#25871

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

…smatch is inferred.

tacaswell · 2023-12-07T16:36:47Z

Thank you for working on this!

Could you also add a test that this warning is in fact raised?

Could you also try adding the repr of the two figures to the warning? I'm not sure it is super helpful right now but might benifit if we improve the figure repr in the future.

zoehcycy · 2023-12-07T19:29:57Z

Sure! I've added figure repr in the warning message. Test details are attached below.

Tested on:
Ubuntu 22.04
Python 3.10.12
IPython : 8.17.2
ipykernel : 6.26.0
ipywidgets : 8.1.1
jupyter_client : 8.5.0
jupyter_core : 5.5.0
jupyter_server : 2.9.1
jupyterlab : 4.0.8
nbclient : 0.8.0
nbconvert : 7.10.0
nbformat : 5.9.2
notebook : 7.0.6
qtconsole : 5.4.4
traitlets : 5.13.0
Test result:

zoehcycy · 2023-12-07T20:13:32Z

Oops, there seem to be a problem. I'll try fixing it.
Plus, I saw that there is actully a procedure for adding tests on GitHub. I'll figure that out too.

jklymak · 2023-12-07T20:38:27Z

lib/matplotlib/figure.py

@@ -1242,6 +1242,12 @@ def colorbar(
            fig.sca(current_ax)
            cax.grid(visible=False, which='both', axis='both')

+        if hasattr(mappable, "figure") and mappable.figure is not self.figure:
+            _log.warning(


Did we want a _log.warning or a warning.warning? How bad do we think it is that a user has put a colorbar on a different figure?

We should probably use warn_external rather that _log.warning.

I think we should also only add this warning if we inferred what axes to steal from

import matplotlib.pyplot as plt fig, ax = plt.subplots() fig2, (axA, axB) = plt.subplots(2) im = ax.imshow([[1, 2], [3, 4]]) fig2.colorbar(im) # this should warn fig2.colorbar(im, ax=ax) # this is weird, but should not warn fig2.colorbar(im, ax=axA) # this should not warn fig2.colorbar(im, cax=axB) # also should not warn

Yes, warn_external is the way to go.

I think we should also only add this warning if we inferred what axes to steal from

I'm not convinced. Generally, it is dubious if the mappable is in another figure than the colorbar, because there's no guarantee that both are shown together. Scenarios I could imagine are somewhat contrieved: (1) you have two figures with images in a paper, but only want to show one colorbar; (2) You have a GUI app and want to render the colorbar in a separate widget. In such cases I would recommend to reconsider whether that's actually a good idea (which the warning would serve to). And if somebody is convinced this is still the right path to take, they can filter the warning.

In turn, it's quite likely that the user has mixed up some fig/ax variables when mappable and colorbar are on different figures, e.g. fig2.colorbar(im, ax=ax) above could very well have been a typo and fig.colorbar(im, ax=ax) was actually intended.

I agree with @timhoffm. I think we should warn anyway, and users can decide if it is a mistake or what they intend to do. For now I'm going to go ahead with this and submit a new commit which also includes some code style fixes and hopefully we can resolve this conversation.

zoehcycy · 2023-12-07T22:27:53Z

Good point. Using `_api.warn_external` is probably more proper as users can change code to stop the warning. As a new contributor I'm not in a position to make the decision though. Can I have your advice on this?

…

On Thu, Dec 7, 2023 at 12:38 PM Jody Klymak ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/matplotlib/figure.py <#27458 (comment)> : > @@ -1242,6 +1242,12 @@ def colorbar( fig.sca(current_ax) cax.grid(visible=False, which='both', axis='both') + if hasattr(mappable, "figure") and mappable.figure is not self.figure: + _log.warning( Did we want a _log.warning or a warning.warning? How bad do we think it is that a user has put a colorbar on a different figure? — Reply to this email directly, view it on GitHub <#27458 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALGAZZGAUR6BCQYYEH766W3YIISM5AVCNFSM6AAAAABAKPQ2FOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZRGA3TAOJQGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

zoehcycy · 2023-12-08T00:50:45Z

Just checked the failure log. Turns out that this new mismatch warning was emitted in test_colorbar_wrong_figure() we had in test_colorbar.py and failed the test.

Should I delete the test test_colorbar_wrong_figure?

=================================== FAILURES ===================================
__________________________ test_colorbar_wrong_figure __________________________
[gw1] linux -- Python 3.9.18 /opt/hostedtoolcache/Python/3.9.18/x64/bin/python

    def test_colorbar_wrong_figure():
        # If we decide in the future to disallow calling colorbar() on the "wrong" figure,
        # just delete this test.
        fig_tl = plt.figure(layout="tight")
        fig_cl = plt.figure(layout="constrained")
        im = fig_cl.add_subplot().imshow([[0, 1]])
        # Make sure this doesn't try to setup a gridspec-controlled colorbar on fig_cl,
        # which would crash CL.
>       fig_tl.colorbar(im)

lib/matplotlib/tests/test_colorbar.py:1231: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
lib/matplotlib/figure.py:1247: in colorbar
    _api.warn_external(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

message = 'Adding colorbar to a different Figure <Figure size 640x480 with 2 Axes> than {repr(self.figure)} which fig.colorbar is called on.'
category = None

    def warn_external(message, category=None):
        """
        `warnings.warn` wrapper that sets *stacklevel* to "outside Matplotlib".
    
        The original emitter of the warning can be obtained by patching this
        function back to `warnings.warn`, i.e. ``_api.warn_external =
        warnings.warn`` (or ``functools.partial(warnings.warn, stacklevel=2)``,
        etc.).
        """
        frame = sys._getframe()
        for stacklevel in itertools.count(1):
            if frame is None:
                # when called in embedded context may hit frame is None
                break
            if not re.match(r"\A(matplotlib|mpl_toolkits)(\Z|\.(?!tests\.))",
                            # Work around sphinx-gallery not setting __name__.
                            frame.f_globals.get("__name__", "")):
                break
            frame = frame.f_back
        # preemptively break reference cycle between locals and the frame
        del frame
>       warnings.warn(message, category, stacklevel)
E       UserWarning: Adding colorbar to a different Figure <Figure size 640x480 with 2 Axes> than {repr(self.figure)} which fig.colorbar is called on.

lib/matplotlib/_api/__init__.py:381: UserWarning

…lleries.

timhoffm · 2023-12-08T18:49:12Z

Should I delete the test test_colorbar_wrong_figure?

no. You should assert that the new warning is thrown using with pytest.warns(…):

timhoffm · 2023-12-08T21:54:15Z

lib/matplotlib/tests/test_colorbar.py

+            with pytest.warns(UserWarning, match="different Figure"):
+                subfig.colorbar(cs, ax=ax, orientation=orientation, fraction=0.4,


Ah, subfigures are an edge case. IMHO we should not warn on different subfigures as long as the colorbar and mappable are in the same top-level figure. To that end, subfigures are more a layout element, and it is still ensured that mappable and colorbar are rendered alongside each other.

I see, thanks for clarifying.

I'd probably do whatever is easier - if you can be more permissive then fine, but warning if the colorbar is in the wrong subfigure is probably fine too - its hard to think of a use case where that would be any more useful than having the colorbar on another figure.

Agreed. But if it's not useful, we should rewrite the test. Doesn't make sense to have a contrived test that needs handling of a warning because of its awkward structure.

Actually, looking at the test, why does it raise the warning? All operation seem to run on the
sub figures individually. AFAICS

we create subplots in the sub figure

we add a contourset to each Axes

Add a colorbar for each contourset, stealing space from the associated Axes

I think the test case warned because I was checking if self.figure is the same instance as mappable.figure in fig.colorbar(mappable), which returned false when:

Both self.figure and mappable.figure are figure instances, but not the same one.

One of them is figure and the other subfigure.

In the latest commit this should be fixed. When self.figure or mappable.figure is a subfigure, I use the associated figure for comparison.

Oh, also, if i'm getting your question correctly, the test case failed in the previous run becasue it did not warn while with pytest.warns(...) is to make sure it actually warns.

zoehcycy · 2023-12-13T18:17:13Z

Hi! Just want to follow up on this. Is there anything else I need to do with this PR?

jklymak · 2023-12-13T21:31:26Z

lib/matplotlib/figure.py

+            if isinstance(mappable_host_fig, mpl.figure.SubFigure):
+                mappable_host_fig = mappable_host_fig.figure
+            if isinstance(self_host_fig, mpl.figure.SubFigure):
+                self_host_fig = self_host_fig.figure


Given our discussion, it seems like this should get a test?

Sure. These lines are for the subfigure cases. In the latest commit, there are tests regarding subfigures in test_figure::test_warn_colorbar_mismatch, line 1685:1702, which specifically include:

Test that no warning is emitted when calling fig.colorbar(subfig), where subfig is associated with the fig.

Test that no warning is emitted when calling subfig1.colorbar(subfig2), where both subfigs are associated with the same figure.

Test that a warning is emitted when calling subfig1.colorbar(subfig2), where subfig1 is not associated with the same figure as subfig2.

Which cases am I missing? Please let me know and I'll add them.

Oh, and the reason L1252 is not covered by test is that self_host_fig is always a figure object no matter if we call colorbar() on a subfigure or figure. I should remove L1251:1252 in the next commit. Thanks!

tacaswell · 2023-12-14T00:16:53Z

I still am 50/50 that we should only warn if we picked the different figure, but will defer to @timhoffm .

zoehcycy · 2023-12-20T03:39:40Z

I still am 50/50 that we should only warn if we picked the different figure, but will defer to @timhoffm .

I'm circling back on this PR, and would like to request review from @timhoffm.

timhoffm

LGTM, modulo one minor stylistic issue.

timhoffm · 2023-12-20T14:20:19Z

lib/matplotlib/figure.py

+        if hasattr(mappable, "figure") and mappable.figure is not None:
+            # Get top level artists
+            mappable_host_fig = mappable.figure
+            self_host_fig = self.figure


I would not declare an extra variable for this. It’s only used once and using self.figure is fine there.

Will fix this.

zoehcycy · 2023-12-20T16:36:50Z

Great. Thanks!

…

On Wed, Dec 20, 2023, 06:22 Tim Hoffmann ***@***.***> wrote: ***@***.**** approved this pull request. LGTM. ------------------------------ In lib/matplotlib/figure.py <#27458 (comment)> : > @@ -1242,6 +1242,20 @@ def colorbar( fig.sca(current_ax) cax.grid(visible=False, which='both', axis='both') + if hasattr(mappable, "figure") and mappable.figure is not None: + # Get top level artists + mappable_host_fig = mappable.figure + self_host_fig = self.figure I would not declare an extra variable for this. It’s only used once and using self.figure is fine there. — Reply to this email directly, view it on GitHub <#27458 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALGAZZEJ6SEX2JR7VN5CM2DYKLYCZAVCNFSM6AAAAABAKPQ2FOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJQHE4TGMBWGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

tacaswell · 2023-12-20T19:14:28Z

Thank you @zoehcycy and congratulations on your first merged Matplotlib PR 🎉 I hope we hear from you again!

I took the liberty of squash-merging this to one commit.

zoehcycy · 2023-12-21T01:25:26Z

No problem. Thanks for everyone's help!

Check if the mappable is in a different Figure than the one fig.color…

d6b6504

…bar is being called on. Ignore this check if the mappable has no host Figure. Warn in case of a mismatch.

github-actions bot reviewed Dec 7, 2023

View reviewed changes

zoehcycy added 2 commits December 7, 2023 01:33

Add figure repr in warning message.

c7aac68

Use _api.warn_external instead of _log.warning. Only warn when the mi…

9339a19

…smatch is inferred.

tacaswell added this to the v3.9.0 milestone Dec 7, 2023

jklymak reviewed Dec 7, 2023

View reviewed changes

zoehcycy added 4 commits December 7, 2023 14:43

Delete test_colorbar::test_colorbar_wrong_figure.

ba97ec5

Warn in all mismatches, inferred or not.

742cd00

Revert deletion of test_colorbar_wrong_figure, with warning test.

78bce33

Linting fixes

10422fd

zoehcycy added 7 commits December 7, 2023 18:11

Catch newly added warning raised in other test cases.

3cbc90a

Catch warning in existing test cases.

2defbed

edge case: subfigure. only warn when top level artist don't match.

c603a4b

flake8 code style check fix

c1d19ec

code style fix.

c14bf18

subfigure test cases revert: no warning.

1c27ec1

Fix edge case: axes.remove(); passing correct pcolormesh object in ga…

6308cb6

…lleries.

timhoffm reviewed Dec 8, 2023

View reviewed changes

zoehcycy requested review from timhoffm, tacaswell and jklymak December 9, 2023 06:17

jklymak reviewed Dec 13, 2023

View reviewed changes

remove redundant lines

ecddddd

code stylistic fix

998a924

timhoffm approved these changes Dec 20, 2023

View reviewed changes

tacaswell merged commit e5562bb into matplotlib:main Dec 20, 2023
40 checks passed

zoehcycy deleted the check-mismatch-colorbar branch December 21, 2023 01:22

rcomer mentioned this pull request Mar 5, 2024

[Bug]: Colorbar cannot be added to another figure #25871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if the mappable is in a different Figure than the one fig.color… #27458

Check if the mappable is in a different Figure than the one fig.color… #27458

zoehcycy commented Dec 7, 2023

zoehcycy commented Dec 7, 2023

github-actions bot left a comment

tacaswell commented Dec 7, 2023

zoehcycy commented Dec 7, 2023

zoehcycy commented Dec 7, 2023

jklymak Dec 7, 2023

tacaswell Dec 7, 2023

timhoffm Dec 7, 2023

zoehcycy Dec 8, 2023

zoehcycy commented Dec 7, 2023 via email

zoehcycy commented Dec 8, 2023 •

edited

timhoffm commented Dec 8, 2023

timhoffm Dec 8, 2023

zoehcycy Dec 8, 2023

jklymak Dec 8, 2023

timhoffm Dec 9, 2023

zoehcycy Dec 9, 2023

zoehcycy Dec 9, 2023

zoehcycy commented Dec 13, 2023

jklymak Dec 13, 2023

zoehcycy Dec 14, 2023

zoehcycy Dec 14, 2023

tacaswell commented Dec 14, 2023

zoehcycy commented Dec 20, 2023

timhoffm left a comment •

edited

timhoffm Dec 20, 2023

zoehcycy Dec 20, 2023

zoehcycy commented Dec 20, 2023 via email

tacaswell commented Dec 20, 2023

zoehcycy commented Dec 21, 2023

		with pytest.warns(UserWarning, match="different Figure"):
		subfig.colorbar(cs, ax=ax, orientation=orientation, fraction=0.4,

Check if the mappable is in a different Figure than the one fig.color… #27458

Check if the mappable is in a different Figure than the one fig.color… #27458

Conversation

zoehcycy commented Dec 7, 2023

PR summary

PR checklist

zoehcycy commented Dec 7, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

tacaswell commented Dec 7, 2023

zoehcycy commented Dec 7, 2023

zoehcycy commented Dec 7, 2023

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

zoehcycy commented Dec 7, 2023 via email

zoehcycy commented Dec 8, 2023 • edited

timhoffm commented Dec 8, 2023

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

zoehcycy commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Dec 14, 2023

zoehcycy commented Dec 20, 2023

timhoffm left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoehcycy commented Dec 20, 2023 via email

tacaswell commented Dec 20, 2023

zoehcycy commented Dec 21, 2023

zoehcycy commented Dec 8, 2023 •

edited

timhoffm left a comment •

edited