Open edmundhenley opened 4 years ago
Hi @edmundhenley! Sorry for taking a little while to get back to you here. Bottom line, this is great!
I like your idea of putting all of the data into a separate file. That is what matplotlib does as well:
https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/_cm_listed.py
At the very bottom of that file they read in the data just as you're proposing to create a new colormap. What would you think of registering the colormap for use in matplotlib right after the data reading in colormaps even?
https://matplotlib.org/3.2.1/api/cm_api.html#matplotlib.cm.register_cmap (call it 'enlil' or something similar?) We would need to import matplotlib within that module and make sure that this all gets taken care of on import of enlilviz right away. Then we could just put ENLIL_CMAP = mpl.cm.get_cmap('enlil')
and all of your new listed data should come over with it.
For tests, I think your suggestion of just picking off a few values should be sufficient for now. That will make sure we can actually load the new cmap data in, and that it is outputting the expected data too.
imagehash would be nice with better images for comparisons, currently I don't have any in there at all, only in the docs. Definitely an area that could be improved! However, for now I'd stick with the low hanging fruit and we can add that in a a separate PR.
Feel free to open a PR and I can comment directly there if this didn't make sense.
Summary
This feature proposal aims to improve compatibility between plots currently produced by operational centres and by enliliviz. Specifically looking at the colormaps used in both. This will aid in plot comparisons, and may help lower (theoretical!) barriers to adoption of enlilviz by operational centres. Feedback welcome - no problem if this only at PR stage!
Rationale
The current enlilviz colormap emulates the rainbow-grey colormap currently used by operational centres. For example the current operational output from NOAA SWPC looks like this:
Very similar to enlilviz's output (e.g. below from #8 - note the different timestamp!)
In terms of implementation, enlilviz achieves this (very!) good approximation, by combining the
gist_ncar
colormap (used for typical background solar wind values) andGreys
colormap (used for enhanced values typically associated with coronal mass ejections).The associated colorbar minimum and maximum values for the solar wind density and speed are set to be
[0, 60]
and[200, 1600]
respectively. I've checked, and thesevmin
/vmax
values match the values used in the operational code - good - nothing further needed on that!However, the
gist_ncar
+Greys
colormap doesn't give a perfect copy of the RGB values used operationally. This is slightly irritating, as impedes like-for-like comparisons, whichThese aren't killer arguments, and it's v likely overkill to assume there's a strong need. However I've now got the set of RGB values used operationally, so not a big extra step to implement them here...
Proposed implementation
I've derived a RGB array for the current operational code. The idea would be to create a
LinearSegmentedColormap
based off this, replacing the currentENLIL_CMAP
This RGB array is long (256x3 entries), so for readability (especially long-term - see below) perhaps better to keep this out ofenlilviz/plotting/plots.py
. Suggest putting this into its own (new) file,enlilviz/plotting/colormaps.py
.This new
colormaps.py
file could contain different functions. These would set the colormap, and perhaps also the vmin/vmax (can't quite decide on whether better to move these here, closer to colormaps, or to leave inplots.py
, with the other global settings values).Currently,
colormaps.py
would contain just one colormap - this new RGB array. In future though, this could be extended to custom colormaps designed to provide optimal (concurrent) discrimination for background solar wind and ejecta values (see e.g. this EOS article kindly shared by @greglucas). In the absence of dynamic 2D plots, this may help operational forecasters better read off data values at a glance, and gain deeper insights into the forecast (and even with dynamic plots, a good default view is helpful, given the time-critical nature of the job!). Hence the suggestion to hive off the colormap intocolormaps.py
: while it may not be too painful to have one 256x3 array inplots.py
, there may be more such arrays in future. So helpful to design up front for that? On flip-side, YAGNI?Proposed tests
Not entirely sure what would be appropriate here. Suggestions:
While 1. seems cool (and am keen to learn how to use imagehash!), suggest more appropriate here to use option 2. for simplicity/avoidance of new dependency. I'll aim for a test using this, unless I hear otherwise.