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

POC: make scaling optional in Collection.get_linestyle#29304

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

Draft
rcomer wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromrcomer:collection-get_linestyle

Conversation

rcomer
Copy link
Member

PR summary

CurrentlyCollection.get_linestyle() returns the scaled dash pattern, i.e. the pattern multiplied by the linewidth. This is inconsistent with the equivalent method onLine2D andPatch (#17316 (comment)). This PR introduces thescaled parameter, to make this behaviour optional. For now I have set the default toFalse to demonstrate that very little seems to be relying on the current scaled behaviour. Obviously we would need a deprecation period before we can do that for real.

As shown by the new test, this allows us to fix the case noted at#28298 (comment). It does not fix the original case from that issue as we just move the error to here:

gc.set_dashes(*self._dash_pattern)

So I think we need some version of#29302 for that case.

PR checklist

@@ -626,6 +626,11 @@ def set_linestyle(self, ls):
':', '', (offset, on-off-seq)}. See `.Line2D.set_linestyle` for a
complete description.
"""
if isinstance(ls, (str, tuple)):
self._original_linestyle = [ls]
Copy link
MemberAuthor

@rcomerrcomerDec 13, 2024
edited
Loading

Choose a reason for hiding this comment

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

I got a lot more failures if I did not make sure this is a list. I note thatCollection.get_facecolor always returns a sequence of colours, having appliedto_rgba_array.

@timhoffm
Copy link
Member

It may make sense to make this part of a general consolidation ofget_linestyle across Artists. There are 3 fundamentally reasonable ways to reoport linstyle back:

  • just return what was input toset_linestyle
  • normalize to a simplified string - all dashes become-- and the actual pattern would have to requested fromget_dashes
  • always normalize to a dash pattern (unscaled)

It seem we do most of this for the different artists#29302 (comment).

I haven't thought too deep, but don't see a clear winner. There are pros and cons for all of them. So it might be an option the add a more generalget_linestyle(mode=) argument to choose the return type. This would reduce current differences to different defaults and open up a relatively pain-free migration path if desired.

Thoughts welcome.

rcomer reacted with thumbs up emoji

@rcomer
Copy link
MemberAuthor

rcomer commentedDec 14, 2024
edited
Loading

I would vote for either getting back what you put in or normalising to unscaled dash pattern. I can’t see the value of normalising to a string because you lose information. The OPs of#23437 and#17316 object to the current Line2D behaviour. Also#23056 (comment).

Normalising to the dash pattern would seem consistent with the fact that colours normalise to RGBA. Presumably it gives some efficiency saving when it is copied across to another object such as a legend handle, as you do not need to translate to the pattern on the new artist.

Of course normalising to unscaled dash pattern is the one that we’re not currently doing anywhere 😄

story645 reacted with thumbs up emoji

@rcomerrcomerforce-pushed thecollection-get_linestyle branch from1eed1a3 tod7513a7CompareDecember 14, 2024 11:34
@github-actionsgithub-actionsbot added the Documentation: examplesfiles in galleries/examples labelDec 14, 2024
@timhoffm
Copy link
Member

Normalising to the dash pattern would seem consistent with the fact that colours normalise to RGBA.

This sounds reasonable, and I'd be in favor of normalizing to a canonical representation. However, I'm not clear that the dash pattern is such a canonical representation in the same way RGBA is for color

  • The dash patten is not intuitive to understand - basically you have to read the documentation.
  • The representation of solid lines and no lines, is not straight forward.
  • Could we have other linestyles in the future that cannot be represented by the dash pattern?

An alternative would be normalizing to

  • 'solid' for solid lines
  • 'none' for no lines
  • a dash pattern for all other cases

Likely 'solid' and 'none' need special checks and code paths anyway, so there's not really a benefit in trying to express a line style without dashes as a formal dash pattern.

@QuLogic
Copy link
Member

This seems like it might be related to#23056, whose discussion never really finished.

timhoffm added a commit to timhoffm/matplotlib that referenced this pull requestDec 18, 2024
Inspired by but independent of the discussion around the representation of linestylesmatplotlib#29304.For other linestyles we have a short visual representation (e.g. "--") and a name (dashed "dashed"). No linestyle currently has four accepted writings "", " ", "none", "None".This PR recommends only to use "" (visual representation) and "none" (named representation). It discourages " " and "None".  This should guide users towards more canonical usage. We may later decide whether to deprecate the alternatives.Note also, that we have not used the alternatives in examples.
timhoffm added a commit to timhoffm/matplotlib that referenced this pull requestDec 18, 2024
Inspired by but independent of the discussion around the representation of linestylesmatplotlib#29304.For other linestyles we have a short visual representation (e.g. "--") and a name (dashed "dashed"). No linestyle currently has four accepted writings "", " ", "none", "None".This PR recommends only to use "" (visual representation) and "none" (named representation). It discourages " " and "None".  This should guide users towards more canonical usage. We may later decide whether to deprecate the alternatives.Note also, that we have not used the alternatives in examples.
@timhoffm
Copy link
Member

timhoffm commentedDec 18, 2024
edited
Loading

Also related:#19300,#17483

It's all quite a mess.

Fundamentally, I believe we need to decide:

  • Do we want a canonical representation of a linestyle?
    I claim yes, at least internally, we want to have a normalized form and not need to care about "-" vs. "solid" vs. ...
  • How does a canonical linestyle representation look like?
    • always a dash pattern (?)
    • 'none' or 'solid' or dash-pattern
    • aLineStyle class
    • ...
  • What do we want to return onget_linestyle()?

I suggest we discuss these on a green field ("What would be a good design") and only afterwards see how we can migrate there.

@greglucas
Copy link
Contributor

FYI that there is this old PR suggesting to make a LineStyle class:#18664

@timhoffm
Copy link
Member

I think we've moved away from necessarily wanting classes for all conecptual types. One of the main arguments was a concise definition of what values are accepted. We can now encode this using the typing system. That's not to say classes are completely off the table. If there's enough related functionality a class is still an option (e.g. the existingMarkerStyle is such a case).

timhoffm added a commit to timhoffm/matplotlib that referenced this pull requestDec 19, 2024
Inspired by but independent of the discussion around the representation of linestylesmatplotlib#29304.For other linestyles we have a short visual representation (e.g. "--") and a name (dashed "dashed"). No linestyle currently has four accepted writings "", " ", "none", "None".This PR recommends only to use "" (visual representation) and "none" (named representation). It discourages " " and "None".  This should guide users towards more canonical usage. We may later decide whether to deprecate the alternatives.Note also, that we have not used the alternatives in examples.
@rcomerrcomer marked this pull request as draftFebruary 8, 2025 13:41
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@rcomer@timhoffm@QuLogic@greglucas

[8]ページ先頭

©2009-2025 Movatter.jp