Skip to content

[ENH]: int / float-tuple like kwarg legend(loc) for rcParams['legend.loc']#25839

Merged
QuLogic merged 1 commit into
matplotlib:mainfrom
NoyHanan:bugfix-for-issue-22338
Jul 15, 2023
Merged

[ENH]: int / float-tuple like kwarg legend(loc) for rcParams['legend.loc']#25839
QuLogic merged 1 commit into
matplotlib:mainfrom
NoyHanan:bugfix-for-issue-22338

Conversation

@NoyHanan

@NoyHanan NoyHanan commented May 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

closes #22338

PR summary

I Added the ability to use int / float-tuple like kwarg legend(loc) for rcParams['legend.loc'], by creating a new validator for rcParams['legend.loc'].
This new change will help to a consistent and a uniform approach to using the legend.
Addresses both parts of the issue

PR checklist

@NoyHanan NoyHanan marked this pull request as ready for review May 10, 2023 12:17
@NoyHanan NoyHanan force-pushed the bugfix-for-issue-22338 branch from 5f21ef5 to 21a74f4 Compare May 11, 2023 05:12
chahak13
chahak13 reviewed May 11, 2023
View reviewed changes
Comment thread lib/matplotlib/rcsetup.py Outdated
@chahak13

chahak13 commented May 11, 2023

Copy link
Copy Markdown
Contributor

Thank you for your first PR 🎉 Looking forward to more such contributions! :)

@NoyHanan

NoyHanan commented May 11, 2023

Copy link
Copy Markdown
Contributor Author

Thank you for your first PR 🎉 Looking forward to more such contributions! :)

Thank you so much!

@NoyHanan

NoyHanan commented May 11, 2023

Copy link
Copy Markdown
Contributor Author

@chahak13 Do I need to note for the API change / add documentation?

chahak13
chahak13 reviewed May 11, 2023
View reviewed changes
Comment thread lib/matplotlib/tests/test_rcparams.py Outdated
@chahak13

chahak13 commented May 11, 2023

Copy link
Copy Markdown
Contributor

There should be a note in the what's new section I believe, yes.

@NoyHanan

NoyHanan commented May 11, 2023

Copy link
Copy Markdown
Contributor Author

There should be a note in the what's new section I believe, yes.

Ok, I'll take care of it tomorrow then.

@tacaswell tacaswell added this to the v3.8.0 milestone May 12, 2023
tacaswell
tacaswell reviewed May 12, 2023
View reviewed changes
Comment thread lib/matplotlib/rcsetup.pyi Outdated
tacaswell
tacaswell reviewed May 12, 2023
View reviewed changes
Comment thread lib/matplotlib/tests/test_rcparams.py Outdated
tacaswell
tacaswell reviewed May 12, 2023
View reviewed changes
Comment thread doc/api/next_api_changes/behavior/22338-NH.rst Outdated
@tacaswell

tacaswell commented May 12, 2023

Copy link
Copy Markdown
Member

Thank you for your work on this @NoyHanan , I think it is headed the right direction! However I left a couple of comments that need to be addressed.

@NoyHanan

NoyHanan commented May 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thank you for your work on this @NoyHanan , I think it is headed the right direction! However I left a couple of comments that need to be addressed.

Hi @tacaswell and @ksunden ,
Thank you for taking the time to review my PR. I appreciate your feedback and will begin working on making the necessary changes.

@tacaswell

tacaswell commented May 16, 2023

Copy link
Copy Markdown
Member

There should also be a test of reading an the values from a file.

@NoyHanan

NoyHanan commented May 16, 2023

Copy link
Copy Markdown
Contributor Author

There should also be a test of reading an the values from a file.

On it.

@NoyHanan

NoyHanan commented May 26, 2023

Copy link
Copy Markdown
Contributor Author

Hi @tacaswell!
I know that you many notifications to attend to, so I wanted to kindly inform you that I have made the necessary adjustments to the code based on your feedback (or at least I believe I have :) ).

@NoyHanan

NoyHanan commented Jun 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @tacaswell! I know that you many notifications to attend to, so I wanted to kindly inform you that I have made the necessary adjustments to the code based on your feedback (or at least I believe I have :) ).

rcomer
rcomer reviewed Jun 18, 2023
View reviewed changes

@rcomer rcomer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for your work on this @NoyHanan, and apologies for the delay in reviewing. Could you update the title of this PR to something more descriptive?

Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/tests/test_rcparams.py
@NoyHanan NoyHanan changed the title Bugfix for issue 22338 [ENH]: int / float-tuple like kwarg legend(loc) for rcParams['legend.loc'] Jun 21, 2023
@NoyHanan

NoyHanan commented Jun 21, 2023

Copy link
Copy Markdown
Contributor Author

Hi @rcomer,
I appreciate your thorough review of my code. I've made the necessary corrections based on your feedback. I'm looking forward to your thoughts on the updates. Thank you!

QuLogic
QuLogic reviewed Jun 24, 2023
View reviewed changes
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
Comment thread doc/users/next_whats_new/rcParams[legend.loc]_supports_float_tuple.rst Outdated
Comment thread doc/users/next_whats_new/rcParams[legend.loc]_supports_float_tuple.rst Outdated
rcomer
rcomer approved these changes Jun 24, 2023
View reviewed changes

@rcomer rcomer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @NoyHanan, your updates look good to me.

@NoyHanan

NoyHanan commented Jun 24, 2023

Copy link
Copy Markdown
Contributor Author

Thanks @NoyHanan, your updates look good to me.

@rcomer thanks alot! a few more things:

  1. I'm commiting the changes @QuLogic, does it need to be approved again?
  2. who may merge the pull request to the main repo?

@rcomer

rcomer commented Jun 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

For code changes, we require approvals from two core developers. So yes, please address @QuLogic's comments. Then once he is happy he will likely approve and merge.

@NoyHanan NoyHanan force-pushed the bugfix-for-issue-22338 branch 2 times, most recently from 9bb4013 to df2ac49 Compare June 24, 2023 21:28
@QuLogic

QuLogic commented Jun 28, 2023

Copy link
Copy Markdown
Member

It looks like you'll also need to update the typing stubs now that the validator is private. Also, you may want to squash the changes together.

@NoyHanan

NoyHanan commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

It looks like you'll also need to update the typing stubs now that the validator is private. Also, you may want to squash the changes together.

Sure, no problem.
Can you explain a bit more about how to update?

@ksunden

ksunden commented Jun 28, 2023

Copy link
Copy Markdown
Member

Just undo the change to the .pyi file

@NoyHanan NoyHanan force-pushed the bugfix-for-issue-22338 branch from df2ac49 to 3e45dd2 Compare June 28, 2023 09:50
@NoyHanan

NoyHanan commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

Just undo the change to the .pyi file

Thanks!

@rcomer

rcomer commented Jun 28, 2023

Copy link
Copy Markdown
Member

To squash, you can do an interactive rebase. I strongly recommend saving a backup copy of your branch first if you have not done this (much) before.

@NoyHanan

NoyHanan commented Jul 8, 2023

Copy link
Copy Markdown
Contributor Author

HI @QuLogic, I'll appriciate your review on the changes I made 🙏

@rcomer

rcomer commented Jul 8, 2023

Copy link
Copy Markdown
Member

@NoyHanan are you happy to squash the commits, or would you prefer us to take care of that?

@NoyHanan

NoyHanan commented Jul 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@NoyHanan are you happy to squash the commits, or would you prefer us to take care of that?

@rcomer I can take care of that, I just thought you guys meant I should squash only the last 3 commits I made for @QuLogic notes.
Should I sqaush them all? and thank you so much for your help and patience.

@rcomer

rcomer commented Jul 8, 2023

Copy link
Copy Markdown
Member

I think minimal numbers of commits are generally preferred.

@NoyHanan NoyHanan force-pushed the bugfix-for-issue-22338 branch from 3e45dd2 to 063c22e Compare July 9, 2023 09:33
@NoyHanan

NoyHanan commented Jul 9, 2023

Copy link
Copy Markdown
Contributor Author

I think minimal numbers of commits are generally preferred.

Thank you for guiding me. I hope that its fine now.

@NoyHanan

NoyHanan commented Jul 15, 2023

Copy link
Copy Markdown
Contributor Author

Hi @QuLogic, could you please review the changes I made? 🙏

@QuLogic QuLogic merged commit c756cbd into matplotlib:main Jul 15, 2023
@rcomer

rcomer commented Jul 17, 2023

Copy link
Copy Markdown
Member

Congratulations on your first PR merged into Matplotlib @NoyHanan! We hope to hear from you again.

@NoyHanan

NoyHanan commented Jul 17, 2023

Copy link
Copy Markdown
Contributor Author

Congratulations on your first PR merged into Matplotlib @NoyHanan! We hope to hear from you again.

Thank you so much, I appreciate your support. You'll definitely hear from me again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell left review comments

@ksunden ksunden ksunden left review comments

@QuLogic QuLogic QuLogic approved these changes

@rcomer rcomer rcomer approved these changes

+1 more reviewer

@chahak13 chahak13 chahak13 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic: rcparams

Projects

First Time Contributors
Status: Merged

Milestone

v3.8.0

Development

Successfully merging this pull request may close these issues.

[Bug]: rcParams['legend.loc'] can't use float-tuple like kwarg legend(loc...)

7 participants

@NoyHanan @chahak13 @tacaswell @rcomer @QuLogic @ksunden @melissawm