Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken inset_locator used with subfigures #24690

Closed
wants to merge 3 commits into from

Conversation

matt256
Copy link
Contributor

@matt256 matt256 commented Dec 10, 2022

PR Summary

Addresses the bug report #24589

From report:

When using inset_locator functions in subfigures, the plots are not placed correctly. Adapting this demo
https://matplotlib.org/stable/gallery/axes_grid1/demo_colorbar_of_inset_axes.html
to use subfigures shows incorrect plots.

Implemented the proposed resolution and wrote a unit test that validates the image referenced in the bug report is generated properly

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@rcomer rcomer changed the base branch from v3.3.x to main December 10, 2022 14:59
@rcomer
Copy link
Member

rcomer commented Dec 10, 2022

Hi @matt256, thanks for the PR and welcome to Matplotlib! It looks like, since you created your branch, there have been some changes on the main branch within the same file you edited. So you will need to rebase and fix some conflicts, following the instructions here:
https://matplotlib.org/devdocs/devel/development_workflow.html#rebasing-on-upstream-main

For your question here, you will see a failure in our tests having both added and replaced an image in the same PR. To fix that, you will need to squash the commits down to one. There are instructions for that here:
https://matplotlib.org/devdocs/devel/development_workflow.html#rewriting-commit-history

@rcomer
Copy link
Member

rcomer commented Dec 12, 2022

Hi @matt256, it looked like your branch got into a bit of a tangle. I thought it might be difficult to guide you through fixing that so, I hope you don't mind, I just pushed a change myself. While I was there, I also dropped your last two commits, as it looked like you used black or a similar auto-formatter on the test file. This created a lot of changes that were not relevant to the problem you are solving so would have made it harder to review. Matplotlib has made a conscious decision to not use black, so formatting needs to be fixed manually. See here for how to check the formatting at the command line.

@matt256
Copy link
Contributor Author

matt256 commented Dec 12, 2022

Hi @rcomer, thanks for you help and digging me out of my hole. You are correct. I got a bit turned around, and I did use black. I'll see if I can get these issues resolved this evening with your guidance on flake8 and then go ahead and get it squashed to get the other error to go away. couple quick questions, though:

  • I noted that creating the test image via a standalone script and creating it inside the test file created a file that was about 3kb different in size. Whatever the change actually was, it caused the image comparison to fail. Because I used the same script to generate the image both inside and outside the test, I assumed it was something that the decorator was doing and it wasn't a big deal, Just wanted to note it, though.
  • This likely may have been due to my late-night stupor, but before I set up my linter and formatter, I was getting flake8 errors for "line too long" throughout the entire test file. Did I see that right? I see that isn't the case anymore. I thought I may have just been the first one to get to it. Am I going crazy? It may just not matter anymore

@jklymak
Copy link
Member

jklymak commented Dec 12, 2022

@matt256 - the default styles all changed with Matplotlib 2.0, but the tests still default to the old style so we did not need to regenerate the images. New test should usually use the up-to-date style: e.g. @image_comparison(['contour_hatching'], remove_text=True, style='mpl20')

@QuLogic
Copy link
Member

QuLogic commented Dec 13, 2022

Please update the title of this PR; it is currently auto-generated from your branch name, but it is not very clear what it is for someone looking at notifications/the list of PRs.

@matt256 matt256 changed the title Axis grid1 patch Fix broken inset_locator used with subfigures Dec 13, 2022
issue test runs but fails comparison

updated styling in test image

bug fix inserted

issue test runs but fails comparison

formatting

fixed long line linter issues
@oscargus
Copy link
Contributor

Closing this as #24783 is a newer version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[Bug]: inset_locator is broken when used with subfigures
5 participants