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

Cleanup initialization in text()#12215

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

Merged
QuLogic merged 1 commit intomatplotlib:masterfromtimhoffm:text-kwargs
Dec 23, 2018

Conversation

timhoffm
Copy link
Member

@timhoffmtimhoffm commentedSep 22, 2018
edited
Loading

PR Summary

Axes.text() created a defaultText and then calledText.update() three times to adapt to the passed kwargs. This is a bit inefficient and cumbersome.

This PR changes the code to create effective kwargs anddirectly pass them to theText() constructor. only update once.

This should not have any effect except being a little faster.

I also had to fixTextWithDash to accept all the kwargs ofText in the constructor. This is a bugfix asTextWithDash claims to be a drop-in replacement forText.

Turns out, makingTextWithDash fully API compatible withText is a bit more cumbersome. Just passing the kwargs on is not enough. There is an issue with the call sequenceTextWithDashes.__init__ ->Text.__init__ ->TextWithDashes.set_transform. I.e. the super init calling a child function before all the attributes of the child are set up.

Options are:

  1. Still delay the kwargs update (but just do one update instead of three).
  2. Resolve the initialization issue. Itshould work ifsuper().__init__() is the last command inTextWithDashes.__init__ instead of the first. However, that seems just like a workaround as well. SeeInit calls subclass method pattern #12220 for further discussion.

For now, I've resorted to the defensive option 1.

@anntzer
Copy link
Contributor

In this specific case, I'd just deprecate TextWithDash in favor of Annotation (possibly after checking that the latter indeed replaces (reasonable...) use cases of the former).

@timhoffm
Copy link
MemberAuthor

Even the scaled back version with a single update does yield different plots with python 3.5 on travis. There's something fishy going on if three sequential updates have a different effect compared to one combined update.

Marking as WIP because this needs further investigation.

@tacaswell
Copy link
Member

Ah, dict iteration order is random in 3.5.

This fix may have to wait until we drop 3.5 (which I suspect will be for the next release, but someone needs to turn the crank on making that decision).

@tacaswelltacaswell added this to theneeds sorting milestoneSep 23, 2018
@timhoffm
Copy link
MemberAuthor

Thanks, that's it.

Looking forward to drop 3.5, then we can also use f-strings 😄.

@timhoffm
Copy link
MemberAuthor

Rebased. Should hopefully work now that were're on Python 3.6+.

Can be squashed.

Copy link
Contributor

@anntzeranntzer left a comment

Choose a reason for hiding this comment

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

modulo ci

@anntzer
Copy link
Contributor

want to squash?

@timhoffm
Copy link
MemberAuthor

Squashed.

@QuLogic
Copy link
Member

So this is still not going to do the full constructor change even on 3.6+?

@timhoffm
Copy link
MemberAuthor

@QuLogic Whst do you mean by „full constructor change“?

This is a basic simplification to just do a single update instead of updating multiple times.

@QuLogic
Copy link
Member

I mean the original description:

This PR changes the code to create effective kwargs and directly pass them to the Text() constructor.

@timhoffm
Copy link
MemberAuthor

Oh, that 😄 (too long ago to remember). Because of

Turns out, making TextWithDash fully API compatible with Text is a bit more cumbersome. Just passing the kwargs on is not enough. There is an issue with the call sequence TextWithDashes.init -> Text.init -> TextWithDashes.set_transform. I.e. the super init calling a child function before all the attributes of the child are set up.

I've restricted myself to

Option 1: Still delay the kwargs update (but just do one update instead of three).

The call sequence issue is not solved with python 3.6+. Also@anntzer proposed to deprecateTextWithDash. This whole topic would need more effort than I'm currently willing to spend on.

TL;DR

No, this won't do the full constructor change.

@timhoffmtimhoffm changed the titleCreate Texts directly with all kwargsCleanup initialization in text()Dec 23, 2018
@timhoffm
Copy link
MemberAuthor

PR title and original description updated.

@QuLogicQuLogic merged commit66b3b83 intomatplotlib:masterDec 23, 2018
@timhoffmtimhoffm deleted the text-kwargs branchDecember 23, 2018 22:42
@anntzeranntzer mentioned this pull requestFeb 28, 2019
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@anntzeranntzeranntzer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

4 participants
@timhoffm@anntzer@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp