-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
base: main
Are you sure you want to change the base?
Conversation
15f37d8
to
9d8c9b6
Compare
lib/matplotlib/collections.py
Outdated
``'-.'`` or ``'dashdot'`` dash-dotted line | ||
``':'`` or ``'dotted'`` dotted line | ||
=========================== ================= | ||
========================================== ================= |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
9d8c9b6
to
bf3585a
Compare
lib/matplotlib/collections.py
Outdated
``'-.'`` or ``'dashdot'`` dash-dotted line | ||
``':'`` or ``'dotted'`` dotted line | ||
=========================== ================= | ||
========================================== ================= |
There was a problem hiding this comment.
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.
21e50c5
to
c9ae7b2
Compare
I have unified the doc-strings and refactored the line style verification a bit, so that all Question: it is "line style" in text, right? (Not "linestyle") Problem: So while now there is no error supplying e.g. I am not really sure if the dash length checks in the backend |
39a1050
to
484b567
Compare
I would also like some input on the best format to normalize/store line-styles. Now, they are normalized to the "short" format |
2430f98
to
dea13f8
Compare
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? |
dea13f8
to
7a90b43
Compare
abd67aa
to
2b055f7
Compare
76c71b0
to
e58b20c
Compare
27a0666
to
632a72c
Compare
A major obstacle to the solution discussed yesterday is that Patches does not have I made a PR to Seaborn mwaskom/seaborn#3400 but got stuck there. All other things seemed to work though. |
To summarize, this PR unifies the handling of line styles so that Earlier Patches and Collections returned the dash pattern for The suggested procedure was to simply add a warning to (This also adds |
ac5b3d3
to
1ae0db6
Compare
To summarize:
For
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.) |
1ae0db6
to
d837a5f
Compare
It also strikes me that there is a difference between "dashes" and "dash pattern", where "dash pattern" is |
8247e61
to
6033dd6
Compare
6033dd6
to
dfb3556
Compare
We should discuss this on a call as soon as 3.9 is branched. |
I find this very unintuitive. I would strongly suggest making Consider the following examples, where the current approach of 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 Suggestion:
|
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 Suggestion:
|
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? |
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
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).