fixed #24617 bug - Added subplot params and figsize getter setter methods to Figure.__init__ with tests #29116
Conversation
…n getter setter for subplot param and figsize
There was a problem hiding this comment.
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.
lib/matplotlib/figure.py
Outdated
| self.set_subplotparams(self.get_subplotparams()) | ||
| self.subplots_adjust(**kwargs) | ||
|
|
||
| def get_subplotparams(self): |
There was a problem hiding this comment.
This method is not run by the tests because you replaced it in the Figure subclass, so only the subclass one gets run by your new test. I'm not too familiar with subplotparams but, as far as I can see, they only exist on a Figure and not on a SubFigure so both these methods could go directly on the Figure class.
There was a problem hiding this comment.
@rcomer Fixed the above inheritance issue. Thanks for pointing it out. I've moved the definitions to Figure and all the pytest cases passed locally. Could you please review my failing pytest macOS tests?
There was a problem hiding this comment.
Those MacOS ones are often flakey. I’ve re-started them to see if they do better this time 🤞
There was a problem hiding this comment.
For the documentation failure I think you just need to add your new methods to an appropriate section here, to make them show up in the docs:
https://github.com/matplotlib/matplotlib/blob/main/doc/api/figure_api.rst
PR summary
This is the first PR I am proposing and this PR fixes #24617 - [ENH]: Add setter/getter methods for all keyword parameters to Figure.init
Few of the arguments passed to Figure.init did not have matching get_{kwarg}/set_{kwarg} methods on Figure (Bug #24617).
This PR focuses on adding getter/setter methods and test coverage for the following:
What problem does this PR solve?
Two of the properties/parameters of Figure class didn't have getter/setter methods implemented - figsize, subplotparams. Without these getter/setter methods, use of the properties in the Figure class is inconsistent and neither does the class provide a way to set properties for these two attributes without the getter/setter methods.
Relevant PRs (Both of these PRs have failing tests and failing checks for PR reviews):
PR checklist:
##Review
Since, this is my first contribution in the open source, I would like to know the feedback and tips to improve my contributions.