oslocyclotronlab / ompy

A python implementation of the Oslo method
https://ompy.readthedocs.io
GNU General Public License v3.0
6 stars 7 forks source link

Fixed depricated changing of cmap #164

Closed vetlewi closed 3 years ago

vetlewi commented 3 years ago

In Matrix.plot() the global cmap is changed to set zero to white. Modifications to global cmap is being deprecated by matplotlib, thus a copy should be made before any changes can be made.

I've added an explicit keyword argument for cmap so that if a user given cmap is given it also will get zero counts set to white. Let me know what you think about it. It might make sense to not do that as well, or have a flag in addition to dictate if zero should be white or not.

A little more context:

WARNING:MainThread:py.warnings:ompy/ompy/matrix.py:329: MatplotlibDeprecationWarning: 
You are modifying the state of a globally registered colormap.
In future versions, you will not be able to modify a registered colormap in-place.
To remove this warning, you can make a copy of the colormap first.
cmap = copy.copy(mpl.cm.get_cmap("viridis"))
fzeiser commented 3 years ago

Couldn't one add a cmap via the kwargs already before?

vetlewi commented 3 years ago

To avoid adding extra arguments one could also just set the default value for cmap in kwargs:

...
current_cmap = copy.copy(cm.get_cmap())
current_cmap.set_bad(color='white')
kwargs.setdefault('cmap', current_cmap)
...
fzeiser commented 3 years ago

To avoid adding extra arguments one could also just set the default value for cmap in kwargs:

...
current_cmap = copy.copy(cm.get_cmap())
current_cmap.set_bad(color='white')
kwargs.setdefault('cmap', current_cmap)
...

Not a big difference, but I like this better. It easily starts to get messy if one provides lots of default kwargs

vetlewi commented 3 years ago

Think ready for merge 👍

fzeiser commented 3 years ago

Yes, just trying to see why Travis is not asked for the check by default

fzeiser commented 3 years ago

Travis is triggered now :+1: I've had issues which I through I had fixed this week but migrated to travis-ci.com now. Maybe this fixed it? Anyhow, thanks for the PR.