-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Do not fail when markers are numpy integers #30838
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
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.
As explained in #30836, I think this is the wrong approach. If we go down this road, next somebody will come along and requests support for pytorch.
jklymak
left a comment
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 are a library that supports Numpy. I don't think this is overly scope-creep to accept numpy integers here.
This is a better expression of intent for a change introduced in commit #2e5f5ff.
Seems justifiable, so I won’t block. Nevertheless I think the numeric markers a a design flaw, and we should work on replacements and discourage their use rather than trying to make them work better for edge cases.
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.
In line with the robustness principle, I think the more liberal Hashable check is appropriate. I don't see an a-priori reason to restrict the types here.
PR summary
Do not fail when marker values are numpy integers e.g.
np.int64(4).The rationale is, matplotlib exists in an ecosystem where pandas and numpy container types are prevalent. When the containers (
pd.Series,np.array) contain only integer values, they do not preserve the originalinttype when the values come out. i.e.ints go innp.int64s come out. With this PR aninttype marker that goes into these containers will still recognised as a marker when it comes out.closes #30836
PR checklist