Skip to content

Allow empty linestyle for collections #23056

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented May 17, 2022

PR Summary

Closes #17316
Closes #22984
Closes #23437
Closes #26784
Helps with #19300

Collections can now have empty linestyles ('', ' ', and 'none', earlier only 'None' was supported). Also, dashes are checked to not be zero and thus avoids a hang in that case.

Will add tests and possibly a release note.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@oscargus oscargus force-pushed the collectionlinestyles branch 5 times, most recently from 15f37d8 to 9d8c9b6 Compare May 26, 2022 14:53
``'-.'`` or ``'dashdot'`` dash-dotted line
``':'`` or ``'dotted'`` dotted line
=========================== =================
========================================== =================
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it would make sense to only have one copy of this, rather than three (each set_linestyle method). No sure what the best way to do that is though. (Nor if it is worthwhile.)

Copy link
Member

Choose a reason for hiding this comment

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

We have the idea of making these concepts explicit so that you could have

linestyles : LineStyle or list thereof

where LineStyle could be turned into a link in the docs.

https://matplotlib.org/devdocs/api/_enums_api.html?highlight=capstyle#module-matplotlib._enums is an attempt, though that has stalled and I'm still not convinced it's the right cut.

For now, I suggest to live with the duplication. Actually, duplication is not that harmful if you rarely need to change the duplicated code; and indirection has its own disadvantages.

@oscargus oscargus force-pushed the collectionlinestyles branch from 9d8c9b6 to bf3585a Compare May 26, 2022 15:02
``'-.'`` or ``'dashdot'`` dash-dotted line
``':'`` or ``'dotted'`` dotted line
=========================== =================
========================================== =================
Copy link
Member

Choose a reason for hiding this comment

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

We have the idea of making these concepts explicit so that you could have

linestyles : LineStyle or list thereof

where LineStyle could be turned into a link in the docs.

https://matplotlib.org/devdocs/api/_enums_api.html?highlight=capstyle#module-matplotlib._enums is an attempt, though that has stalled and I'm still not convinced it's the right cut.

For now, I suggest to live with the duplication. Actually, duplication is not that harmful if you rarely need to change the duplicated code; and indirection has its own disadvantages.

@oscargus oscargus force-pushed the collectionlinestyles branch 2 times, most recently from 21e50c5 to c9ae7b2 Compare May 27, 2022 10:21
@oscargus
Copy link
Member Author

I have unified the doc-strings and refactored the line style verification a bit, so that all set_linestyle methods rely on the same code.

Question: it is "line style" in text, right? (Not "linestyle")

Problem: Collection does not actually support a way of having an empty line style as they store dash-patterns. The only way to currently get that is to set the line width to zero or the edge color to 'none'. Would it make sense to trick an empty line style by setting e.g. the line with to zero? Lines do store both _linestyle and _dashpattern, but Collection only _linestyle (always as a dash pattern).

So while now there is no error supplying e.g. ' ' as line style for a Collection (e.g. scatter plot) it still is not really supported.

I am not really sure if the dash length checks in the backend set_dashes is still required now? Is this supposed to be a user facing method? Currently, every time set_linewidth or set_dashes is called, there will be a check, so the only way (I can figure out) to get a hang is to set _linestyle or _dashpattern in a stupid way.

@oscargus oscargus force-pushed the collectionlinestyles branch 2 times, most recently from 39a1050 to 484b567 Compare May 27, 2022 10:28
@oscargus
Copy link
Member Author

I would also like some input on the best format to normalize/store line-styles. Now, they are normalized to the "short" format '-', '--', '-.', ':', and 'none', but I think that the rcparam validator returns 'solid', 'dashed', ... 'none'. So which is the preferred way?

@oscargus oscargus force-pushed the collectionlinestyles branch 2 times, most recently from 2430f98 to dea13f8 Compare May 27, 2022 11:45
@oscargus
Copy link
Member Author

I guess the proper way to actually handle empty line styles in collections is to separate the line style and the dashes as in lines. Does anyone foresee any obvious drawbacks of this?

@oscargus oscargus force-pushed the collectionlinestyles branch 2 times, most recently from 76c71b0 to e58b20c Compare July 21, 2022 18:44
@oscargus
Copy link
Member Author

A major obstacle to the solution discussed yesterday is that Patches does not have get_dashes currently. Hence, it is not really possible to use the "warn on get_linestyle and say they should use get_dashes instead" approach as that will require a pin of the MPL version.

I made a PR to Seaborn mwaskom/seaborn#3400 but got stuck there. All other things seemed to work though.

@oscargus
Copy link
Member Author

oscargus commented Jun 23, 2023

To summarize, this PR unifies the handling of line styles so that get_linestyle returns a string of the line style pattern and get_dashes returns the dash pattern. It also unified the checking of line styles and dash patterns so that the same requirements are set on all, independent of class.

Earlier Patches and Collections returned the dash pattern for get_linestyle. Collections have aliased dashes so that it is currently returned there as well, but Patches does not have a get_dashes method.

The suggested procedure was to simply add a warning to Collection.get_linestyle for a few version stating that the output will change and the dash pattern info can be obtained through get_dashes. However, I forgot that the same applies to Patches, but there is no get_dashes for that.

(This also adds get_dashes to Lines and Patches. Earlier it was not possible to obtain the dash pattern for Line2D.)

@oscargus oscargus force-pushed the collectionlinestyles branch 2 times, most recently from ac5b3d3 to 1ae0db6 Compare July 9, 2023 05:05
@oscargus
Copy link
Member Author

oscargus commented Jul 9, 2023

To summarize:

Patch.get_linestyle earlier returned exactly what was passed as linestyle. Now, get_linestyle emits a warning that it will change, but I have no idea what is a good way to turn it off. (In the long run, get_linestyle should return the normalized line style string and get_dashes, the dash pattern.)

Collection.get_linestyle did always return the dash pattern. Now, this warns that it will change and that get_dashes (which has long existed) can be used to get the same result.

For Line2D and Patch, a get_dashes method is added which can be used to get the dash pattern.

I do not get the mypy error. Probably one would like to type get_dashes to not return LineStyleType? Or rather add LineStyleString and DashPatternType and combine those into LineStyleType for set_linestyle and then add the correct for get_dashes and later on get_linestyle?

It also strikes me that set_dashes is no longer valid for Collection. Should it be added as set_dashes = set_linestyle or should it be a method that actually only sets the dash pattern (and therefore making more sense, but also making it impossible to do `col.set_dashes('-')'?

What is the preferred way of normalized line styles? The "named" or the "symbols"? Also, which string should be used for an empty line style? (Right now it varies.)

@oscargus oscargus force-pushed the collectionlinestyles branch from 1ae0db6 to d837a5f Compare July 9, 2023 05:36
@oscargus
Copy link
Member Author

oscargus commented Jul 9, 2023

It also strikes me that there is a difference between "dashes" and "dash pattern", where "dash pattern" is (offset, dashes). Hence, set_dashes and get_dashes are not reciprocal. Should there be a get_dash_pattern method? If so, this will cause some issues with Collection, which currently returns the dash pattern with get_dashes (and get_linestyle),

@oscargus oscargus force-pushed the collectionlinestyles branch 2 times, most recently from 8247e61 to 6033dd6 Compare July 9, 2023 08:57
@tacaswell
Copy link
Member

We should discuss this on a call as soon as 3.9 is branched.

@eltos
Copy link

eltos commented Sep 4, 2024

@oscargus

To summarize, this PR unifies the handling of line styles so that get_linestyle returns a string of the line style pattern and get_dashes returns the dash pattern.
[...]
For Line2D and Patch, a get_dashes method is added which can be used to get the dash pattern.

I find this very unintuitive. I would strongly suggest making get_linestyle consistently return an exact representation of the actual line style, not a lossy string representation (i.e. adopt Line2D behaviour to Collections, not the other way round)!

Consider the following examples, where the current approach of get_linestyle causes very bad user experience:

from matplotlib import pyplot as plt
plt.figure(figsize=(3,2))
line1, = plt.plot([0,1],[0,3], ls=(0, [1,2,1,2,10,2]))
line2, = plt.plot([0,1],[0,2])
line3, = plt.plot([0,1],[0,1])


line2.set_linestyle(line1.get_linestyle())
# user now expects both lines to have identical linestyles,
# but because get_linestyle returns "--" this is not the case - very unexpected behaviour

ls = (0, [1,2])
line3.set_linestyle(ls)
print(line3.get_linestyle())
# prints "--" which is not the set value but a valid different linestyle - very unexpected

if line1.get_linestyle() == line2.get_linestyle() == line2.get_linestyle()
    ... # this is true even though all lines have completely different linestyles - very unexpected

grafik

Suggestion:

  • Make get_linestyle consistently return exactly what was set as linestyle,
    such that it actually represents the linestyle correctly and line2.set_linestyle(line1.get_linestyle()) works as expected
  • Optionally:
    • Keep the proposed get_dashes to return the dash pattern even if linestyle was set as string
    • Add get_linestyle_string to return the string representation (even if linestyle was set as on-off pattern). Because not all linestyles can be represented as string, the method may either raise an exception or have a special return value like --- (which is not a valid linestyle!) to distinguish between exact and approximate representations. However, I don't actually think such a method is need.

@eltos
Copy link

eltos commented Sep 4, 2024

line2.set_linestyle(line1.get_linestyle())

If I'm not mistaken, with this PR using

line2.set_linestyle(line1.get_dashes())
# or the identical
line2.set_dashes(line1.get_dashes())

would also not work correctly, because get_dashes returns the scaled _dash_pattern and not the _unscaled_dash_pattern, but set_dashes = set_linestyle expects an unscaled pattern and applies the scaling (again (and again (and again))).

Suggestion:

  • make get_dashes return the _unscaled_dash_pattern instead, because the scaling is internal and should not be exposed to the user.

@timhoffm
Copy link
Member

timhoffm commented Sep 5, 2024

This does a lot of things around linestyles/dashes. I have to say, I have lost what exactly we do here. There seems to be some internal code cleanup, some fixes, some additions, and some are breaking API changes. The latter seems controversial. Can we untangle the topics?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants