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

Fix for importing mpl colormaps with recent versions of matplotlib #1077

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

ejeschke
Copy link
Owner

@ejeschke ejeschke commented Dec 2, 2023

Fixes an issue with the API in recent versions of matplotlib.

Ginga can continue to import and use matplotlib colormaps.

@ejeschke ejeschke added maintenance Work done to keep code maintained widget and removed documentation labels Dec 2, 2023
@ejeschke ejeschke added this to the 5.0 milestone Dec 2, 2023
@ejeschke ejeschke self-assigned this Dec 2, 2023
ginga/cmap.py Outdated
@@ -13309,8 +13309,7 @@ def add_matplotlib_cmap(cm, name=None):
def add_matplotlib_cmaps(fail_on_import_error=True):
"""Add all matplotlib colormaps."""
try:
import matplotlib.pyplot as plt
from matplotlib import cm as _cm
from matplotlib import colormaps as _mpl_cm
if MPL_LT_3_4:
from matplotlib.cbook import mplDeprecation as MatplotlibDeprecationWarning
else:
Copy link
Owner Author

Choose a reason for hiding this comment

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

@pllim, should we drop support for mpl < 3.4 and git rid of this workaround needed for suppressing MatplotlibDeprecationWarning ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Matplotlib is at 3.8 now. Matplotlib 3.3 has not been touched since early 2021. Do you still have any users stuck with matplotlib < 3.4? If not, probably less work to drop it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sounds good. I'll remove the support from this module for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should also bump this?

matplotlib>=2.1

Copy link
Owner Author

Choose a reason for hiding this comment

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

good catch

@ejeschke
Copy link
Owner Author

ejeschke commented Dec 2, 2023

Here is a simple one if you have a minute or two only, @pllim.

ginga/cmap.py Outdated
except ImportError:
if fail_on_import_error:
raise
# silently fail
return

# NOTE: Update if matplotlib has new public API for this.
for name in plt.colormaps():
for name in _mpl_cm:
if not isinstance(name, str):
continue
try:
# Do not load deprecated colormaps
with warnings.catch_warnings():
warnings.simplefilter('error', MatplotlibDeprecationWarning)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the deprecation warning catching should be removed. It should no longer happen.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@pllim, because those particular color maps have been removed some time ago? I guess we can add the code back if they deprecate more color maps in the future. Wondering why they deprecated color maps...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ejeschke , I don't follow matplotlib development closely, so not sure. But I do not see any warning now using the matplotlib I have locally. Do you?

I guess does not hurt to keep it as well.

Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

LGTM FFTM TYVM 😸

@ejeschke ejeschke merged commit 646a68f into ejeschke:main Dec 4, 2023
9 checks passed
@ejeschke
Copy link
Owner Author

ejeschke commented Dec 4, 2023

TY@PLLIM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Work done to keep code maintained widget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants