FIX: savefig)...,transparent=True) now makes inset_axes transparent a… #22816

sramesh750 · 2022-04-10T01:50:01Z

PR Summary

This PR makes inset_axes in save fig(...,transparent=True) function transparent in addition to the parent axes.
Addresses issue #22674

PR Checklist

Tests and Styling

[ x ] Has pytest style unit tests (and pytest passes).
[ x ] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. 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.

tacaswell · 2022-04-11T22:22:50Z

Thank you for working on this @sramesh750 .

The implementation looks reasonable to me, however it needs at least one test so we can be sure that the issues does not comeback.

Looking at this, I also suspect that we failing to set sub-figures (and their Axes) to be transparent. Would you be willing to also address that issue while you are working on this (and thinking about the right way to test it ;)) ?

sramesh750 · 2022-04-13T21:15:39Z

@tacaswell Am I supposed to commit the result_images file that was created when running the tests? I am currently unsure of why I have some checks that are failing, such as the AppVeyor build.

tacaswell · 2022-04-14T12:38:13Z

Appveyor failing is unrelated (a new release of a jlab extension had a file who's name is too long for the windows version in the appveyor image).

sramesh750 · 2022-04-14T19:42:35Z

@tacaswell Thank you for all the help! Is there anything else I need to do in order to get this pull request approved?

tacaswell · 2022-04-14T21:09:59Z

I do not fully understand the fully transparent image? I guess before you would see the outlines of the sub-figures? However, as it stands the test is not great because if you deleted the body of the test function it would still pass!

sramesh750 · 2022-04-15T18:08:58Z

I believe this new test should not cause false positives since I added visible subplots.

tacaswell · 2022-04-15T19:02:51Z

Can you squash this down to one commit? I expect this to fail because you added and then changed the image file in this PR (we only want the good version in the history!).

If you are not comfortable doing the squashing we can do that for you.

sramesh750 · 2022-04-15T19:26:40Z

I'm not as comfortable with squashing, so I would appreciate if you could take care of that!

anntzer · 2022-04-16T10:07:08Z

lib/matplotlib/tests/test_figure.py

@@ -557,6 +557,26 @@ def test_savefig_pixel_ratio(backend):
    assert ratio1 == ratio2


+@image_comparison(['transparent_inset_axes'],
+                    extensions=['png'], savefig_kwarg={'transparent': True})


please use remove_text=True to remove the ticklabels (this makes the images more robust against underlying changes of the library.

please combine the two tests together (by putting the inset_axes into an axes in a subfigure).

Other than that, this looks good to me.

(don't forget to remove the now unneeded images)

anntzer

Just needs a squash first.

sramesh750 · 2022-04-16T19:36:44Z

I'm not too familiar with squashing. Which commits need to be squashed, or is this something that can be done for me?

anntzer · 2022-04-16T22:27:45Z

Done.

sramesh750 · 2022-04-16T22:38:45Z

Thank you so much!

oscargus · 2022-04-18T10:10:01Z

Just to make sure: as mentioned in the original issue it may be that one not always would like this. Is there a way to make the "main figure" transparent, but not the inset?

Should there be a release note for this? Some users may rely on the current behavior.

(In the long run one probably would like the inset to not show content from the main axes that happens to end up inside the inset area.)

jklymak

Are you sure the figure returns to previous settings if subfigures are used?

The docstring needs to be updated.

To @oscargus comment, a) this is pretty new, b) if folks want to toggle transparency of individual elements, they can just not use the transparent kwarg. OTOH, I could imagine a situation where it would be a breaking change for the transparency to be applied to the inset axes, and it would ruin a figure. (imagine a line plot inset over an image).

jklymak · 2022-04-18T10:33:23Z

lib/matplotlib/figure.py

@@ -3061,10 +3061,18 @@ def savefig(self, fname, *, transparent=None, **kwargs):
            if transparent:
                kwargs.setdefault('facecolor', 'none')
                kwargs.setdefault('edgecolor', 'none')
+                for subfig in self.subfigs:
+                    subfig.set_facecolor('none')


I don't think this gets reversed when the savefig is completed?

Oopsie, good catch.

anntzer · 2022-04-18T11:19:47Z

lib/matplotlib/figure.py

@@ -3061,10 +3061,18 @@ def savefig(self, fname, *, transparent=None, **kwargs):
            if transparent:
                kwargs.setdefault('facecolor', 'none')
                kwargs.setdefault('edgecolor', 'none')
+                for subfig in self.subfigs:
+                    subfig.set_facecolor('none')


Oopsie, good catch.

sramesh750 · 2022-04-18T14:53:48Z

@jklymak could you please clarify in more detail what changes need to be made? I am a bit confused on the functionality that needs to be added. Do we want users to be able to make the figure transparent but not the subfigure? Also a bit confused what you mean by "this gets reversed". Thank you so much!

jklymak · 2022-04-18T15:48:43Z

The transparent kwarg should only affect the figure during printing. Once the printing context is left, the figure should revert to whatever state it was in before the print started. Here you are losing the original state and just changing the subfigure facecolor.

sramesh750 · 2022-04-18T21:02:39Z

I updated the code, but I will need help with squashing the commit again.

Fixed

tacaswell · 2022-04-18T23:07:10Z

@sramesh750 I fixed this by dropping your merge commit and then cherry-picking the new commit.

The issue is that when you updated locally Antony had squashed all of your commit into one which, despite having identical changes, git viewed as different from your local branch (which to be fair it is a different history!). Because there were some commits on github and some commits on your local system, when you added your last commit and tried to push GH helpfully said "I can not do that because the histories do not match! Try running this command to fix it: git merge ... " which in general is helpful and correct. However in this case it re-introduced the commits we were trying to get rid of into the history.

What you want to do to prevent this in the future is

git remote update
git remote -v   # to check the name of your remote for the next line
git reset --hard YOUR_REMOTE/my-feature

before you do any more work.

🤦🏻 I now see that this also has to be squashed...doing that too.

tacaswell · 2022-04-18T23:16:37Z

I feel like I keep moving the goal posts, but I think this will only work as expected for 1 level of sub-figures (that is sub-figure that have sub-figures that have axes that have insets will not behave as expected). I think we can fix that by pulling the loops out into a (locally defined) function that looks something like

def _recursively_make_transparent(exit_stack, subfig):
     exit_stack.enter_context(subfig, ....)
     for ax in subfig.axes:
        ...
     for sub_subfig is subfig.subfigs:
        _recursively_make_transparent(exit_stack, sub_subfig)

I'm fine with this going in as-is as an improvement and opening an issue for the more general case or having it done here if you are feeling up to it @sramesh750 !

sramesh750 · 2022-04-19T00:30:24Z

@tacaswell I am unfortunately busy in the next few weeks, so for now I think it would be best to merge these improvements and open up a new issue for the more general case. If no one gets onto that new issue, I will definitely be interested in working on it as soon as I can!

oscargus · 2022-04-19T07:15:22Z

@jklymak OK! Makes sense.

I was more thinking of the case where one puts an inset over a plotted line to illustrate e.g. a detail of the plot. Currently, one can simply rely on the transparent kwarg, but with this one need to change approach (although still doable).

Considering it is recent and this does not seem to be considered a problem, I will not argue against it. (One can also argue that the current PR is probably more "correct" than the earlier behavior.)

Just wanted to make sure that the potential drawback was considered.

tacaswell · 2022-04-19T13:55:10Z

@oscargus fair, I see both sides, but lean towards making all backgrounds transparent as it is a simpler rule (no special cases).

QuLogic · 2022-04-26T05:52:29Z

lib/matplotlib/figure.py

@@ -3061,10 +3061,24 @@ def savefig(self, fname, *, transparent=None, **kwargs):
            if transparent:
                kwargs.setdefault('facecolor', 'none')
                kwargs.setdefault('edgecolor', 'none')
+                # set subfigure to appear transparent in printed image
+                for subfig in self.subfigs:


Subfigures can be nested arbitrarily; this only handles one level.

I think another issue was supposed to be opened to handle this general case.

The nesting should be handled here.

@tacaswell noted how to do this recursively here #22816 (comment)
This could also be done with a stack approach:

subfigs = [*self.subfigs] while subfigs: this_subfig = subfigs.pop(0) subfigs.extend(this_subfig.subfigs) # Do something with this_subfig

On a side note (and not for this PR), I wonder if we want some kind of self.iter_subfigures or similar that would do the above iteration with a callable.

jklymak · 2022-06-02T14:29:18Z

I'm a little confused about the status of this one... @anntzer you approved, but its not showing as approved, so not clear if you still have questions?

Actually I think this can be done without a new baseline image?

anntzer · 2022-06-10T17:09:47Z

lib/matplotlib/tests/test_figure.py

+    fig.add_subfigure(gs[0:2, 1])
+    fig.add_subplot(gs[0:2, 1])
+    ax2 = plt.gca()
+    ax2.inset_axes([.1, .2, .3, .4])


I think you can add ax.spines[:].set_visible(False); ax.set(xticks=[], yticks=[]) to all axes instantiations, which effectively makes them invisible (and thus the entire savefig should be invisible). Then you can check_figures_equal with a fully empty plot.

jklymak · 2022-06-23T08:18:53Z

This still needs a bit of revision. Moving to draft until addressed...

timhoffm · 2022-10-16T11:48:29Z

@sramesh750 do you still plan to work on this? If not, one of the core devs or somebody else could take over.

tacaswell

The recurniveness of sub-figures needs to be handled.

tacaswell · 2022-12-16T20:40:45Z

Pushing this to 3.8

chahak13 · 2022-12-21T09:37:58Z

@sramesh750 do you plan to work on this? If not, I'd like to pick up this PR to finish the work.

sramesh750 · 2022-12-31T14:23:31Z

@sramesh750 do you plan to work on this? If not, I'd like to pick up this PR to finish the work.

@chahak13 No, you can finish the work if you'd like

QuLogic · 2023-02-24T21:10:05Z

This has been replaced by #24816.

github-actions bot reviewed Apr 10, 2022

View reviewed changes

tacaswell added this to the v3.6.0 milestone Apr 11, 2022

anntzer requested changes Apr 16, 2022

View reviewed changes

anntzer approved these changes Apr 16, 2022

View reviewed changes

anntzer force-pushed the my-feature branch from 169451d to cc70a86 Compare April 16, 2022 22:27

jklymak previously requested changes Apr 18, 2022

View reviewed changes

anntzer previously requested changes Apr 18, 2022

View reviewed changes

tacaswell force-pushed the my-feature branch from 63e442b to 94fefd5 Compare April 18, 2022 23:00

FIX: savefig(...,transparent=True) now makes inset_axes transparent

e64af6d

tacaswell force-pushed the my-feature branch from 94fefd5 to e64af6d Compare April 18, 2022 23:08

tacaswell approved these changes Apr 19, 2022

View reviewed changes

QuLogic reviewed Apr 26, 2022

View reviewed changes

jklymak requested a review from anntzer June 2, 2022 14:29

anntzer previously approved these changes Jun 10, 2022

View reviewed changes

anntzer reviewed Jun 10, 2022

View reviewed changes

jklymak marked this pull request as draft June 23, 2022 08:18

QuLogic modified the milestones: v3.6.0, v3.7.0 Aug 18, 2022

tacaswell requested changes Dec 16, 2022

View reviewed changes

tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 16, 2022

chahak13 mentioned this pull request Dec 26, 2022

[FIX]: Make inset axes transparent on savefig(..., transparent=True) #24816

Merged

4 tasks

github-actions bot added the status: needs rebase label Jan 13, 2023

QuLogic closed this Feb 24, 2023

FIX: savefig)...,transparent=True) now makes inset_axes transparent a… #22816

FIX: savefig)...,transparent=True) now makes inset_axes transparent a… #22816

Conversation

sramesh750 commented Apr 10, 2022

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

tacaswell commented Apr 11, 2022

sramesh750 commented Apr 13, 2022

tacaswell commented Apr 14, 2022

sramesh750 commented Apr 14, 2022

tacaswell commented Apr 14, 2022

sramesh750 commented Apr 15, 2022

tacaswell commented Apr 15, 2022

sramesh750 commented Apr 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer left a comment

Choose a reason for hiding this comment

sramesh750 commented Apr 16, 2022

anntzer commented Apr 16, 2022

sramesh750 commented Apr 16, 2022

oscargus commented Apr 18, 2022

jklymak left a 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

sramesh750 commented Apr 18, 2022

jklymak commented Apr 18, 2022

sramesh750 commented Apr 18, 2022

tacaswell commented Apr 18, 2022

tacaswell commented Apr 18, 2022

sramesh750 commented Apr 19, 2022

oscargus commented Apr 19, 2022

tacaswell commented Apr 19, 2022

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

jklymak commented Jun 2, 2022

Choose a reason for hiding this comment

jklymak commented Jun 23, 2022

timhoffm commented Oct 16, 2022

tacaswell left a comment

Choose a reason for hiding this comment

tacaswell commented Dec 16, 2022

chahak13 commented Dec 21, 2022

sramesh750 commented Dec 31, 2022

QuLogic commented Feb 24, 2023