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

Added logic to apply wspace and hspace in subfigures without constrained layout #25646

Closed

Conversation

dswelch
Copy link

@dswelch dswelch commented Apr 7, 2023

PR Summary

This is a PR for #25511

The wspace and hspace parameters in subfigures() did not have any affect when a constrained layout was not used.
The function _redo_transform_rel_fig in figure.py seemed to disregard the wspace and hspace parameters, so I added logic that would apply the correct spacing, based off of what I saw when using a constrained layout.

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

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rcomer
Copy link
Member

rcomer commented Apr 8, 2023

Thanks @dswelch. Have you checked what would happen if you had more than two rows or columns? From a quick look at the code, I think the outer rows/columns might end up wider than the middle ones.

I’m also wondering if there’s a reason we can’t just use the subplotspec’s get_position method.

@dswelch
Copy link
Author

dswelch commented Apr 10, 2023

Hi @rcomer, thank you for the feedback! You're correct that it does not work in the case of more than two rows or columns. Could you elaborate where I would use the get_position method? When experimenting with it by just returning the results of get_position, I got these results, with the constrained layout on the left and non constrained layout on the right:
Screenshot 2023-04-10 at 3 00 01 PM

This is the same example that I used from the original issue, with wspace and hspace set to 0.2. I don't believe I fully understand what this function does so any guidance would be appreciated.

@rcomer
Copy link
Member

rcomer commented Apr 11, 2023

Thanks for checking.

As far as I can tell, the get_position method is designed to do what we need except for subplots rather than subfigures. However, I had missed that the underlying GridSpec.get_grid_positions relies on using Figure.subplotpars to get the left, right, wspace, etc. properties. So that does tie it to subplots, and is the “reason we can’t just use the subplotspec’s get_position method.”

@rcomer
Copy link
Member

rcomer commented May 6, 2023

I just looked again at this and I realised that GridSpec.get_grid_positions doesn't have to rely on Figure.subplotpars provided the relevant attributes are set on the gridspec itself.

subplotpars.update(**{k: getattr(self, k) for k in self._AllowedKeys})

So perhaps using get_grid_positions would work, but only if we originally set up the gridspec to not have any white space around the edges. I.e. pass left=0, right=1, bottom=0, top=1 here:

gs = GridSpec(nrows=nrows, ncols=ncols, figure=self,
wspace=wspace, hspace=hspace,
width_ratios=width_ratios,
height_ratios=height_ratios)

@dswelch are you still interested in working on this and if so do you want to try that? I can't guarantee it will work!

@dswelch
Copy link
Author

dswelch commented May 15, 2023

@rcomer I'm sorry for the late response, I would like to open up this issue to others if possible. If there is anything I should do to make that easier please let me know.

@rcomer
Copy link
Member

rcomer commented May 18, 2023

Thanks for letting us know @dswelch. I’ll add the “orphan” label, then someone might adopt it.

@rcomer
Copy link
Member

rcomer commented Jun 4, 2023

After some more discussion at #25511 I opened #25960 with a slightly different approach. So I will close this one in favour of that. Thank you for your work on this @dswelch. This one turned out to be somewhat thorny problem. I hope that that has not put you off and that we hear from you again some time.

@rcomer rcomer closed this Jun 4, 2023
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.

None yet

2 participants