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

an attempt to fix register_cmap import in plotting #533

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

golobor
Copy link
Member

@golobor golobor commented Aug 9, 2024

addresses #520

@golobor golobor requested a review from Phlya August 9, 2024 08:30
@golobor
Copy link
Member Author

golobor commented Aug 9, 2024

@Phlya , i've requested your review, b/c you seem to have fixed #520 before (dc1fdd9) , but the fix didn't work for me. Did your fix work for you?

I think the reason your version doesn't work for me is b/c matplotlib.colormaps is not a module, but a variable.
Here is where it gets imported into the matplotlib namespace from mpl.cm:
https://github.com/matplotlib/matplotlib/blob/857e7d8aacd52d552e2e617ce23da30166419dcd/lib/matplotlib/__init__.py#L1515
And here is where it gets defined:
https://github.com/matplotlib/matplotlib/blob/857e7d8aacd52d552e2e617ce23da30166419dcd/lib/matplotlib/cm.py#L238

In any case, the new version should be agnostic to whether mpl.colormaps is a module or a variable. Would you green-light it?

@Phlya
Copy link
Member

Phlya commented Aug 9, 2024

Did they also swap the argument order? WTF. OK, looks good to me, thanks for fixing it again!

@golobor golobor merged commit f608f25 into master Aug 9, 2024
5 checks passed
@golobor golobor deleted the fix_register_cmap_import branch August 9, 2024 14:58
@golobor golobor restored the fix_register_cmap_import branch August 9, 2024 15:01
@golobor
Copy link
Member Author

golobor commented Aug 9, 2024

ah, good catch re: argument order. I will use kwargs to make sure that it's backward compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants