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 usage of Matplotlib color maps #33491

Closed
mwageringel opened this issue Mar 11, 2022 · 13 comments
Closed

fix usage of Matplotlib color maps #33491

mwageringel opened this issue Mar 11, 2022 · 13 comments

Comments

@mwageringel
Copy link

Matplotlib used to have a global dictionary of color maps. By now, it is considered internal API and does not contain newer color maps anymore, so some of them cannot be used in Sage:

sage: density_plot(x, (-1,1), (-1,1), cmap='turbo')
/usr/lib/python3.10/site-packages/sage/repl/rich_output/display_manager.py:608: RichReprWarning: Exception in _rich_repr_ while displaying object:
Color map turbo not known (type import matplotlib.cm; matplotlib.cm.datad.keys() for valid names)

CC: @tscrim @davidlowryduda

Component: graphics

Keywords: matplotlib

Author: Markus Wageringel

Branch: 2022ed6

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/33491

@mwageringel mwageringel added this to the sage-9.6 milestone Mar 11, 2022
@mwageringel
Copy link
Author

Branch: u/gh-mwageringel/33491

@mwageringel
Copy link
Author

Commit: 2022ed6

@mwageringel
Copy link
Author

Author: Markus Wageringel

@mwageringel
Copy link
Author

comment:1

This branch obtains the colormaps from matplotlib.colormaps instead of matplotlib.cm.

I have removed the module-wide cm variable as I do not see a purpose of it. The code is more than 10 years old, so it is probably not related to modularization. My best guess is that it has to do with lazy imports in Python 2.

In the long run, we can also consider removing Sage's Colormaps collection and replace it by Matplotlib functionality, since Matplotlib already has support for registering new color maps.


New commits:

2022ed633491: fix usage of matplotlib colormaps

@tscrim
Copy link
Collaborator

tscrim commented Mar 12, 2022

comment:2

I agree that there is no reason to nail that in memory. This is a good step towards removing redundancy in Sage that can be achieved elsewhere. LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Mar 12, 2022

Reviewer: Travis Scrimshaw

@mwageringel
Copy link
Author

comment:3

Thank you.

@vbraun
Copy link
Member

vbraun commented Mar 27, 2022

Changed branch from u/gh-mwageringel/33491 to 2022ed6

@tobiasdiez
Copy link
Contributor

Changed commit from 2022ed6 to none

@tobiasdiez
Copy link
Contributor

comment:5

On conda, one sees now the following error

sage.repl.rich_output.display_manager.RichReprWarning: Exception in _rich_repr_ while displaying object: cannot import name 'colormaps' from 'matplotlib' (/usr/share/miniconda/envs/sage-build/lib/python3.8/site-packages/matplotlib/__init__.py)

I think this may be introduced by this ticket, see https://github.com/sagemath/sage/runs/5800737359?check_suite_focus=true. (Not sure if it's only conda-related).

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 4, 2022

comment:6

Sounds like build/pkgs/matplotlib/install-requires.txt should have been adjusted?
It says matplotlib >=3.3.1

@davidlowryduda
Copy link
Contributor

comment:7

I think you're right. This seems to require matplotlib >= 3.5.0 (https://matplotlib.org/stable/users/prev_whats_new/whats_new_3.5.0.html#id13).

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 4, 2022

comment:8

I've opened #33642 for this

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

No branches or pull requests

6 participants