cctbx / cctbx_project

Computational Crystallography Toolbox
https://cci.lbl.gov/docs/cctbx
Other
210 stars 111 forks source link

Matplotlib colormap deprecation #894

Closed Baharis closed 1 year ago

Baharis commented 1 year ago

According to matplotlib issue #20853 the interface for colormaps in matplotlib (mpl) changes. Method mpl.cm.get_cmap('name') is deprecated in version 3.7 and will be removed in version 3.9, probably in a year or so. Instead, mpl.colormap.get_cmap('name') or mpl.colormap['name'] is encouraged. plt.get_cmap() is expected to remain available.

This is but a very minor issue for cctbx.xfel and other areas in active development. As they continue to use the most recent matplotlib version, the problem can be simply circumvented by refactoring all code to use the new syntax.

Nonetheless, this made me think about legacy software and codebase supported by cctbx. In their case, refactoring the code might break any functionality plotting with gradients, as the new syntax was introduced in matplotlib version 3.4.0, barely two years ago (Mar 26, 2021). If the current codebase should indeed support older applications such as the ones using python2, it needs to allow access to the current syntax existing back from matplotlib version 2.2 from June 28, 2018.

irisdyoung commented 1 year ago

Yep -- I think we have a fair number of try-excepts built into cctbx for cases like what you describe, and that's what I'd recommend here as well. It's a little clunky but does seem to do the job.

Baharis commented 1 year ago

I agree, although quite frankly I'd just prefer the Python2 support to be dropped. @irisdyoung, are you aware of any place in the code, for example in libtbx, where such aliases are stored centrally? Just thinking about scattering try-excepts in every place in cctbx where colormaps are used makes me feel physically ill.

irisdyoung commented 1 year ago

I guess the closest to what you're hoping for would be how many files import from six.moves (as a result of the py2 -> py3 move and hypothetically maintaining backward compatibility). We don't have a centralized location of our own for this, to my knowledge. I think it's a good idea though, with the caveat that I wouldn't want to end up with a ton of unrelated code in one place. What about defining some plotting functionality in a central location, including colormaps? Some plotting utilities could be really handy anyway.

Baharis commented 1 year ago

I feel like libtbx already acts as such centralised location; for example see the code for libtbx.mpi4py whose behavior changes with environment. I had an idea to define colormaps there as from libtbx.matplotlib import colormaps where colormap is conditionally cm or colormaps. This is still a rough patch, albeit maybe a little better one. If libtbx in not an appropriate location for this, then maybe we could have libtbx.moves? Just an idea.

irisdyoung commented 1 year ago

Great, I endorse this. Will defer to someone closer to libtbx to formally approve adding this.

phyy-nx commented 1 year ago

Hi all, cctbx.xfel depends on packages which have no python 2 support so I believe the argument about maintaining python2 is moot. At this point we are mostly keeping python3 syntax out of cctbx_project/xfel for convenience in passing our code checkers, and indeed most of the exceptions in py2_syntax_exceptions.txt are from cctbx.xfel.

Outside of cctbx_project/xfel, I see only plt.get_cmap is used. Therefore, @Baharis rather than implementing a libtbx.matplotlib shim, I recommend updating any XFEL code to the new matplotlib API. Thanks!

Baharis commented 1 year ago

AFAIK the plt.get_cmap is a syntax discouraged by matplotlib compared to mpl.colormap['name'], but it is safe across all matplotlib versions. I'll see about updating all the code in xfel to either one of those supported versions, probably the first one (though I find the latter more readable).

Baharis commented 1 year ago

There are only a few places where cm.get_cmap needs to be updated, see #897. However, I have found multiple places where colormaps are accessed directly, e.g. matplotlib.cm.jet. I tried inquiring about it but it remains unclear to me whether this syntax is encouraged and expected to stay. It might be better in a long run to refactor those as well.