Skip to content
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

Support Cn, n>9 in plot() shorthand format. #27943

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 18, 2024

PR summary

Closes #27828.

PR checklist

@ksunden
Copy link
Member

ksunden commented Mar 18, 2024

It is a corner case that I'm not too sure we are worried about, but does induce a change in behavior:

plot([2, 5, 1], "-C11") is currently interpreted as ls=-, c=C1, ms=1 ("tri-down" marker... which is unfortunately represented by a digit) 1,2,3,4, (tri in each direction) and 8 (octagon) each have markers associated with their string representations, so it is not only tri-down (1).

With this PR it will be interpreted as a solid line of color C11, with no marker.

A workaround would be to specify the marker anywhere other than immediately after the color specification (1-C1, -1C1, C1-1 should all work).

I will also note that C11 goes through the fast path and does not get a marker, so there is already some inconsistency if you are using the tri-down marker which induces order dependence. (And so this only matters in the case where you also want a line and not just markers)

How concerned are we with this case?

Personally I'm of the opinion that color cycles are allowed to be longer than 10 colors, and we should support specifying them here, and thus I think I'm okay with it, but do want it acknowledged. (I'm also wondering if that change of behavior, however slight, requires a release note)

Other than the behavior change, the implementation looks good.

@timhoffm
Copy link
Member

I’m ok with the change. The old interpretation of “C11” is not intuitive. Ideally, people will already have used a different order, at least I would have done that. But anyway it’s quite a niche case and not worth blocking the CN extension.

@tacaswell
Copy link
Member

It needs a clear API change not, but I'm also onboard with this change.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 18, 2024

Added API change note.

@ksunden ksunden added this to the v3.9.0 milestone Mar 20, 2024
@ksunden ksunden merged commit 1174aad into matplotlib:main Mar 20, 2024
43 checks passed
@anntzer anntzer deleted the plotc10 branch March 20, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ".C10" does not work as plot shorthand format spec
5 participants