Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
fixed #24617 bug - Added subplot params and figsize getter setter methods to Figure.__init__ with tests#29116
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…n getter setter for subplot param and figsize
There was a problem hiding this 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 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 uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This method is not run by the tests because you replaced it in theFigure 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 aFigure and not on aSubFigure so both these methods could go directly on theFigure class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For the documentation failure Ithink 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 PRfixes#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.