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

ListedColormap: don't pass N colors #9811

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

mathause
Copy link
Collaborator

  • Closes #xxxx
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

matplotlib/matplotlib#29135 deprecated passing N to ListedColormap. This PR avoids the deprecation warnings once the new matplotlib version is released.

@mathause mathause added the run-upstream Run upstream CI label Nov 22, 2024
@mathause
Copy link
Collaborator Author

That's actually also mentioned in #9810 (maybe because internal warnings now error?)

Copy link
Collaborator Author

@mathause mathause left a comment

Choose a reason for hiding this comment

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

This should get us back the the previous behaviour. Maybe except for handling of gray values. mpl argues that lists of colors should not be duplicated/ truncated except for countour plots. I did not have the patience to go into that.

https://github.com/matplotlib/matplotlib/blob/84464dd085210fb57cc2419f0d4c0235391d97e6/lib/matplotlib/colors.py#L1212

@@ -1806,7 +1806,6 @@ def test_colors_np_levels(self) -> None:
cmap = artist.cmap
assert isinstance(cmap, mpl.colors.ListedColormap)
colors = cmap.colors
assert isinstance(colors, list)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's now an numpy array. I am reasonably sure that is a behaviour change in matplotlib and I don't think we should try to preserve that.

https://github.com/matplotlib/matplotlib/blame/84464dd085210fb57cc2419f0d4c0235391d97e6/lib/matplotlib/contour.py#L190

Copy link
Collaborator

@headtr1ck headtr1ck Nov 22, 2024

Choose a reason for hiding this comment

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

I think the instance check was only there to make mypy happy. We don't really care what type colors is, as long as it is indexable.

@mathause
Copy link
Collaborator Author

remaining failures are unrelated

Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@dcherian
Copy link
Contributor

Thanks for taking this on!

@dcherian dcherian merged commit b18afbf into pydata:main Nov 22, 2024
27 of 29 checks passed
@mathause mathause deleted the listed_colormap_no_n branch November 25, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-upstream Run upstream CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants