Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

POC: make scaling optional in Collection.get_linestyle #29304

wants to merge 1 commit into from
+33 −15

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Dec 13, 2024

PR summary

Currently Collection.get_linestyle() returns the scaled dash pattern, i.e. the pattern multiplied by the linewidth. This is inconsistent with the equivalent method on Line2D and Patch (#17316 (comment)). This PR introduces the scaled parameter, to make this behaviour optional. For now I have set the default to False 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:

matplotlib/lib/matplotlib/patches.py

Line 603 in a23ef5a

gc.set_dashes(*self._dash_pattern)

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

PR checklist

@rcomer rcomer force-pushed the collection-get_linestyle branch from d8f3119 to 1eed1a3 Compare December 13, 2024 20:34
rcomer
rcomer commented Dec 13, 2024
View reviewed changes
lib/matplotlib/collections.py
@@ -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
Member Author

@rcomer rcomer Dec 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 that Collection.get_facecolor always returns a sequence of colours, having applied to_rgba_array.

@timhoffm
Copy link
Member

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

  • just return what was input to set_linestyle
  • normalize to a simplified string - all dashes become -- and the actual pattern would have to requested from get_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 general get_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
Copy link
Member Author

rcomer commented Dec 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 😄

@rcomer rcomer force-pushed the collection-get_linestyle branch from 1eed1a3 to d7513a7 Compare December 14, 2024 11:34
@github-actions github-actions bot added the Documentation: examples files in galleries/examples label Dec 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

QuLogic commented Dec 18, 2024

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 request Dec 18, 2024
@timhoffm
Discourage alternate strings for 'none' linestyle
2d39143 
Inspired by but independent of the discussion around the representation of linestyles matplotlib#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 request Dec 18, 2024
@timhoffm
Discourage alternate strings for 'none' linestyle
39c609f 
Inspired by but independent of the discussion around the representation of linestyles matplotlib#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 timhoffm mentioned this pull request Dec 18, 2024
Merged
@timhoffm
Copy link
Member

timhoffm commented Dec 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
    • a LineStyle class
    • ...
  • What do we want to return on get_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 existing MarkerStyle is such a case).

timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Dec 19, 2024
@timhoffm
Discourage alternate strings for 'none' linestyle
55320d3 
Inspired by but independent of the discussion around the representation of linestyles matplotlib#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.
@rcomer rcomer marked this pull request as draft February 8, 2025 13:41
@timhoffm timhoffm mentioned this pull request Feb 28, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
API: changes API: consistency Documentation: examples files in galleries/examples status: needs comment/discussion needs consensus on next step status: needs rebase topic: collections and mappables topic: legend topic: mplot3d
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rcomer @timhoffm @QuLogic @greglucas