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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
oscargus wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromoscargus:collectionlinestyles

Conversation

oscargus
Copy link
Member

@oscargusoscargus commentedMay 17, 2022
edited
Loading

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 (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

@oscargusoscargusforce-pushed thecollectionlinestyles branch 5 times, most recently from15f37d8 to9d8c9b6CompareMay 26, 2022 14:53
``'-.'`` or ``'dashdot'`` dash-dotted line
``':'`` or ``'dotted'`` dotted line
=========================== =================
========================================== =================
Copy link
MemberAuthor

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 (eachset_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.

``'-.'`` 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.

@oscargusoscargusforce-pushed thecollectionlinestyles branch 2 times, most recently from21e50c5 toc9ae7b2CompareMay 27, 2022 10:21
@oscargus
Copy link
MemberAuthor

I have unified the doc-strings and refactored the line style verification a bit, so that allset_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, butCollection only_linestyle (always as a dash pattern).

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

I am not really sure if the dash length checks in the backendset_dashes is still required now? Is this supposed to be a user facing method? Currently, every timeset_linewidth orset_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.

@oscargusoscargusforce-pushed thecollectionlinestyles branch 2 times, most recently from39a1050 to484b567CompareMay 27, 2022 10:28
@oscargus
Copy link
MemberAuthor

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?

@oscargusoscargusforce-pushed thecollectionlinestyles branch 2 times, most recently from2430f98 todea13f8CompareMay 27, 2022 11:45
@oscargus
Copy link
MemberAuthor

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?

@oscargusoscargusforce-pushed thecollectionlinestyles branch 2 times, most recently from76c71b0 toe58b20cCompareJuly 21, 2022 18:44
@oscargus
Copy link
MemberAuthor

A major obstacle to the solution discussed yesterday is that Patches does not haveget_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 Seabornmwaskom/seaborn#3400 but got stuck there. All other things seemed to work though.

@oscargus
Copy link
MemberAuthor

oscargus commentedJun 23, 2023
edited
Loading

To summarize, this PR unifies the handling of line styles so thatget_linestyle returns a string of the line style pattern andget_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 forget_linestyle. Collections have aliaseddashes so that it is currently returned there as well, but Patches does not have aget_dashes method.

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

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

eltos reacted with thumbs down emoji

@oscargusoscargusforce-pushed thecollectionlinestyles branch 2 times, most recently fromac5b3d3 to1ae0db6CompareJuly 9, 2023 05:05
@oscargus
Copy link
MemberAuthor

oscargus commentedJul 9, 2023
edited
Loading

To summarize:

Patch.get_linestyle earlier returned exactly what was passed aslinestyle. 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 andget_dashes, the dash pattern.)

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

ForLine2D andPatch, aget_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 typeget_dashes to not returnLineStyleType? Or rather addLineStyleString andDashPatternType and combine those intoLineStyleType forset_linestyle and then add the correct forget_dashes and later onget_linestyle?

It also strikes me thatset_dashes is no longer valid forCollection. Should it be added asset_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
Copy link
MemberAuthor

oscargus commentedJul 9, 2023
edited
Loading

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

@oscargusoscargusforce-pushed thecollectionlinestyles branch 2 times, most recently from8247e61 to6033dd6CompareJuly 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 commentedSep 4, 2024
edited
Loading

@oscargus

To summarize, this PR unifies the handling of line styles so thatget_linestyle returns a string of the line style pattern andget_dashes returns the dash pattern.
[...]
ForLine2D andPatch, aget_dashes method is added which can be used to get the dash pattern.

I find this very unintuitive. I would strongly suggest makingget_linestyle consistently return anexact representation of theactual 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 ofget_linestyle causesvery bad user experience:

frommatplotlibimportpyplotaspltplt.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 behaviourls= (0, [1,2])line3.set_linestyle(ls)print(line3.get_linestyle())# prints "--" which is not the set value but a valid different linestyle - very unexpectedifline1.get_linestyle()==line2.get_linestyle()==line2.get_linestyle()    ...# this is true even though all lines have completely different linestyles - very unexpected

grafik

Suggestion:

  • Makeget_linestyle consistently return exactly what was set as linestyle,
    such that it actually represents the linestyle correctly andline2.set_linestyle(line1.get_linestyle()) works as expected
  • Optionally:
    • Keep the proposedget_dashes to return the dash pattern even if linestyle was set as string
    • Addget_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

line2.set_linestyle(line1.get_linestyle())

If I'm not mistaken, with this PR using

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

would also not work correctly, becauseget_dashes returns the scaled_dash_pattern and not the_unscaled_dash_pattern, butset_dashes = set_linestyle expects an unscaled pattern and applies the scaling (again (and again (and again))).

Suggestion:

  • makeget_dashes return the_unscaled_dash_pattern instead, because the scaling is internal and should not be exposed to the user.

@timhoffm
Copy link
Member

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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
future releases
7 participants
@oscargus@jklymak@tacaswell@eltos@timhoffm@QuLogic@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp