Skip to content

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

Closed
dswelch wants to merge 3 commits into
matplotlib:mainfrom
dswelch:wspace_and_hspace_in_subfigures
Closed

Added logic to apply wspace and hspace in subfigures without constrained layout#25646
dswelch wants to merge 3 commits into
matplotlib:mainfrom
dswelch:wspace_and_hspace_in_subfigures

Conversation

@dswelch

@dswelch dswelch commented Apr 7, 2023

Copy link
Copy Markdown

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

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

rcomer commented Apr 8, 2023

Copy link
Copy Markdown
Member

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

dswelch commented Apr 10, 2023

Copy link
Copy Markdown
Author

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

rcomer commented Apr 11, 2023

Copy link
Copy Markdown
Member

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

rcomer commented May 6, 2023

Copy link
Copy Markdown
Member

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

dswelch commented May 15, 2023

Copy link
Copy Markdown
Author

@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

rcomer commented May 18, 2023

Copy link
Copy Markdown
Member

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

@rcomer

rcomer commented Jun 4, 2023

Copy link
Copy Markdown
Member

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.

2 participants