Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Changed return type of get_{x,y}ticklabels to plain list#17028

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

Conversation

SidharthBansal
Copy link
Contributor

PR Summary

Changed return type of get_{x,y}ticklabels to plain list
Fixes#16885

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@SidharthBansal
Copy link
ContributorAuthor

@anntzer@timhoffm another way will be to change

defget_minorticklabels(self):
and
defget_majorticklabels(self):
to list. But that is used in
returnself.zaxis.get_majorticklabels()

So, I think we have only one option to typecast it. Any suggestions?

@tacaswelltacaswell added this to thev3.3.0 milestoneApr 4, 2020
@tacaswell
Copy link
Member

Thanks for working on this! In addition to the actually changes this will also need a note inhttps://github.com/matplotlib/matplotlib/blob/master/doc/api/next_api_changes/behaviour.rst

@tacaswell
Copy link
Member

I see the case for changing this is either Axes or Axis. If we are going to do it to x and y, the same should be done for z (inconsistency is one of the most frustrating things for users and one of our biggest problems).

Doing it inAxis fixes it everywhere all, but changing it inAxes is a smaller change.

On balance, given that this api change is primarily on the repr not the coding side (it still has the chance to break people if they are relying on doing anything but looking with their eyes at stdout) I think the "make things as consistent as possible" wins over "make the smallest change you can" so please make the change in Axis.

@SidharthBansal
Copy link
ContributorAuthor

Thanks for working on this! In addition to the actually changes this will also need a note inhttps://github.com/matplotlib/matplotlib/blob/master/doc/api/next_api_changes/behaviour.rst

I will be happy to make there changes once#17021 is merged. Otherwise it will result in conflicts. So, waiting for it to be merged.

@tacaswell
Copy link
Member

Dealing with merge conflicts / rebases is (for better or worse) a normal part of the workflow on Matplotlib :/ . It is also possible that, so long as there are no over lapping changes to the contents, git will handle the rename smoothly.

@SidharthBansal
Copy link
ContributorAuthor

Ok. Let me do it now then.


`get_xticklabels` and `get_yticklabels` now returns plain list
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Previously, get_xticklabels and get_yticklabels returns silent_list.
Copy link
Member

Choose a reason for hiding this comment

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

Can enumerate all of the methods that will be changed? The audience for this will not have the context you current have. What are the consequences of this change? Who is likely to be affected? What can they do to get the old behavior back?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I have two questions in my mind. 1st next_api_changes are the changes in the upcoming release which has not been shipped to market yet. Please correct me if I am wrong.
2nd: behaviour.rst is meant for knowing the changes across releases. Correct me if I am wrong.
thanks

@tacaswell
Copy link
Member

I am :+1 in principle, but I think the API change notes needs a bit more context.

SidharthBansal reacted with thumbs up emoji

@anntzer
Copy link
Contributor

Per#16903 (review) the change should be made on the Axis method, not the Axes wrappers.

SidharthBansal reacted with thumbs up emoji

@anntzeranntzer removed their request for reviewApril 4, 2020 22:45
@SidharthBansal
Copy link
ContributorAuthor

44 tests are failing due to indirectly/directly usingAxis methods(get_minorticklabels ORget_majorticklabels). I was worried about this initially(#17028 (comment)) so I did changes on get_{x,y}ticklabels. Shall I now proceed with these changes and change 44 tests OR shall I roll back to changes of get_{x,y}ticklabels?

@anntzer
Copy link
Contributor

You need to change the constructor call (the list constructor doesn't take a name, unlike the silent_list one).

@SidharthBansal
Copy link
ContributorAuthor

Done! Kindly review. Thanks

@@ -1171,14 +1171,14 @@ def get_majorticklabels(self):
ticks = self.get_major_ticks()
labels1 = [tick.label1 for tick in ticks if tick.label1.get_visible()]
labels2 = [tick.label2 for tick in ticks if tick.label2.get_visible()]
returncbook.silent_list('Text major ticklabel',labels1 + labels2)
returnlist(labels1 + labels2)
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't even need thelist call here;labels1 + labels2 is already a list.
same below.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

yeah, my mistake. corrected!

@SidharthBansalSidharthBansalforce-pushed thechange_return_type_to_normal_list branch fromb9e9b41 to352f688CompareApril 7, 2020 11:00
@SidharthBansal
Copy link
ContributorAuthor

Rebased

@SidharthBansalSidharthBansalforce-pushed thechange_return_type_to_normal_list branch from14ca83b to352f688CompareApril 7, 2020 11:37
Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

You're finding out the nitpicky nature of reStructuredText 😄. I've provided some hints how to placate Sphinx.

SidharthBansal reacted with thumbs up emoji
SidharthBansaland others added4 commitsApril 8, 2020 04:52
Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@SidharthBansal
Copy link
ContributorAuthor

Yeah, sphinx is easy to understand. Varied syntaxes and an uncountable number of new things I am learning here :-)

timhoffm reacted with laugh emoji

@SidharthBansal
Copy link
ContributorAuthor

So, Circle CI is for Sphinx?
Completed the suggested changes

@timhoffm
Copy link
Member

So, Circle CI is for Sphinx?

Yes, we run our doc builds on CircleCI. Note also that you can view the built docs via the respective link in the checks overview list:

image

@SidharthBansal
Copy link
ContributorAuthor

That's great! Thanks

@SidharthBansal
Copy link
ContributorAuthor

I thinkneeds rebase label could be removed now

@timhoffmtimhoffm merged commit9daaa7d intomatplotlib:masterApr 13, 2020
@timhoffm
Copy link
Member

Thanks@SidharthBansal!

I‘ve squashed the commits before merging. We try to keep the commits reasonably focused to keep our history clean. Since you plan to contribute more, I encouraged you to organize your PRs directly into one or a few semantically separated commits. It‘s ok and common practice to force-push to your feature branch for the fixes and incremental updates.

@SidharthBansal
Copy link
ContributorAuthor

I will abide by the advice. Thanks!
Yeah, I will continue to contribute. Right now, I am learning packaging and proposal related stuff. So that I can start with that work asap. :)

timhoffm reacted with thumbs up emoji

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

@anntzeranntzeranntzer left review comments

@tacaswelltacaswelltacaswell approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Change return type get_{x,y}ticklabels to plain list
4 participants
@SidharthBansal@tacaswell@anntzer@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp