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

Getters setters #25901

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Getters setters #25901

wants to merge 8 commits into from

Conversation

dutta712
Copy link

PR summary

Hello, this is my first open source contribution to a project. This pull request addresses the concerns raised in #24617 which was cherry-picked from @garveyz2's PR which was then tagged as orphaned.

Please guide me through any mistake which I've made, any missing files I needed to commit etc. I'm trying to learn contributing to open source and even though I've basically copied the entire solution, I feel like I've learnt a lot in that process.

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.

Copy link
Contributor

@melissawm melissawm left a comment

Choose a reason for hiding this comment

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

Hi @dutta712! Thanks for your PR - first we need to get the tests passing, starting with flake8. I've added a few comments to help, but after this I think there are other stuff you will need to take a look at. Let me know if you need help understanding the test failures. Cheers!

lib/matplotlib/figure.py Outdated Show resolved Hide resolved
lib/matplotlib/figure.py Show resolved Hide resolved
lib/matplotlib/figure.py Show resolved Hide resolved
lib/matplotlib/figure.py Show resolved Hide resolved
@dutta712
Copy link
Author

Hello @melissawm, I was thinking since shaw5lee#1 was already merged, I can revert the changes and only commit the changes regarding

  • use alias mechanism for size_inches -> figsize
  • use alias mechanism from layout -> layout_engine

And also I don't understand why my tests failed as I haven't added anything extra I just copied the solutions from an orphaned PR whose tests had passed #24696.

@melissawm
Copy link
Contributor

A few of the errors are typing errors detected by Mypy and I don't think you need to worry about them at this time. But the linting errors pointed above need to be fixed first. Then, you can click on Details right next to the failing error to get a full traceback, and that is usually enough to tell you what's failing.

@dutta712
Copy link
Author

Thank you @melissawm for the comments, they really helped. I went ahead and fixed some of the sphinx issues as well but I'm not understanding this warning from the sphinx report

lib/matplotlib/figure.py:docstring of matplotlib.figure.FigureBase.set_subplotparams:14: WARNING: Block quote ends without a blank line; unexpected unindent.

After I solve the sphinx issues I will try and understand the other issues as well.

Should I commit the changes I've done so far?
And thanks again for helping me, it's really helping me learn.

@melissawm
Copy link
Contributor

melissawm commented May 25, 2023

Are you seeing those errors locally, after fixing the previous items? I'd recommend commiting the initial fixes and seeing what happens. That message could be a consequence of not fixing the initial linting errors I pointed above, as I don't see anything too obvious beyond that in terms of docstring formatting on your changes here.

@dutta712
Copy link
Author

Okay I did a small error. I mistakenly deleted a "_" from "_api". I undid the change locally and fixed some whitespace in blank line. I am going to recommit those change now. My apologies.

@melissawm
Copy link
Contributor

No need to apologize! Now, we are seeing something else here: https://github.com/matplotlib/matplotlib/actions/runs/5091243063/jobs/9151011765?pr=25901#step:6:1 Can you take a look and eliminating the spurious whitespace characters?

I can see there is another failure here: https://github.com/matplotlib/matplotlib/actions/runs/5091243063/jobs/9151011765?pr=25901#step:6:52 Maybe something else that got renamed.

Thanks!

@dutta712
Copy link
Author

I have fixed and commited those changes. Most of the tests which had failed have passed. Some of which are still failing I'm not exactly sure why. Can you give some suggestions @melissawm?

Thank you!

@melissawm
Copy link
Contributor

It looks like you have a failing test here: https://github.com/matplotlib/matplotlib/actions/runs/5092639244/jobs/9154253571?pr=25901#step:13:328

First step is to investigate whether this has to do with your changes.

@dutta712
Copy link
Author

Hi @melissawm thanks for the suggestions.
After going through the failed tests again and again, I've found that a few errors are due to

  • figure.pyi not having the updated interfaces which I'm trying to implement/find from previous PRs.
  • And also I did not include the appropriate tests with test_figure.py which I've found in a previous PR.

So, I'm working on these two issues at the moment and will commit to see what other test cases are/will fail. Thanks!

@dutta712
Copy link
Author

dutta712 commented Jun 2, 2023

Hello @melissawm, as I can see the latest commits have quite a few errors. The errors I've boiled down to

  • a few functions lacking stub files in figure.pyi
  • AttributeError: 'Figure' object has no attribute 'get_subplotpars'. Did you mean: 'get_subplotparams'? this error because the tests require the function to be named as get_subplotpars instead of get_subplotparams.

Apart from these am I missing some other tests? Thanks!
How long will these keep failing ;_;

@dutta712 dutta712 closed this Jun 2, 2023
@dutta712 dutta712 reopened this Jun 2, 2023
matplotlib.figure.Figure.subplots_adjust
matplotlib.figure.Figure.get_subplotparams
"""
return self.subplotpars
Copy link
Contributor

Choose a reason for hiding this comment

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

The test is failing because of this line, I don't think you've defined what subplotpars is.

@dutta712
Copy link
Author

dutta712 commented Jun 2, 2023

What I've found is that the original implementation (ref #21549) of get/set Subplotpars that my original commit referenced from had some values changed. So, I'll look into the original commit and copy the entire implementation which will probably fix most of the issues. And also some of the other methods also have stub files missing so I will implement those too.

@rcomer
Copy link
Member

rcomer commented Jul 15, 2023

Hi @dutta712 are you still interested in working on this one?

@dutta712
Copy link
Author

@rcomer Apologies for my absence, felt a little dejected after all the updates kept getting error. I will restart the work.

@QuLogic
Copy link
Member

QuLogic commented Aug 1, 2023

We are holding a (New contributors meeting](https://hackmd.io/@matplotlib/ncm2023) tomorrow (Aug 1) at 1700 UTC; if you would like some help or to discuss contribution issues, please join us.

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

Successfully merging this pull request may close these issues.

None yet

5 participants