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

[Feature?]: improve compatibility between operational and enlilviz colormaps #11

Open
edmundhenley opened this issue Jun 8, 2020 · 1 comment

Comments

@edmundhenley
Copy link

edmundhenley commented Jun 8, 2020

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:
EnlilSwpc_2020-06-08Z0755
Very similar to enlilviz's output (e.g. below from #8 - note the different timestamp!)
enlilviz_PR8

In terms of implementation, enlilviz achieves this (very!) good approximation, by combining the gist_ncar colormap (used for typical background solar wind values) and Greys 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 these vmin/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, which

  • may impede devs' examinations of other issues (e.g. blockier nature of enlilviz 2D plots, possibly addressed by Adding an enhancement feature #8) - harder to separate effects of different colormaps and different interpolations than just different interpolations
  • may (assertion without justification!) impede adoption by intended users (forecasters, their own users) if any of these have strong requirements for new plots (from "must look reasonably similar: current OK; new cmap not needed" to "must look pixel-wise identical [???]: current not OK; new cmap - and more - needed")

These 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 current ENLIL_CMAP
This RGB array is long (256x3 entries), so for readability (especially long-term - see below) perhaps better to keep this out of enlilviz/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 in plots.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 into colormaps.py: while it may not be too painful to have one 256x3 array in plots.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:

  1. At the deep end, perhaps a colorhash (using imagehash) on an artefact (full-size .png of colormap only - no other matplotlib-dependent issues - tick length etc). This test would catch unwanted changes to the RGB array. This however adds a dependency...
  2. Simpler: printing RGB values, and comparing to static values in the test - would achieve similar to above?
  3. Other I've not thought of?

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.

@greglucas
Copy link
Member

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.

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

No branches or pull requests

2 participants