Resolve #60, #56 - Allow multiple aliases, update colormaps from colorcet.m (second attempt)#63
Resolve #60, #56 - Allow multiple aliases, update colormaps from colorcet.m (second attempt)#63randallpittman wants to merge 20 commits intoholoviz:masterfrom
Conversation
|
After a lot of second-guessing I think I'm going to press the proverbial big red button now. Hopefully no more commits now! |
|
Thanks for all your hard work here! I'll review it as soon as I get a chance. |
jbednar
left a comment
There was a problem hiding this comment.
Looks great! I've made scattered comments below, but there are two main issues:
- Is it possible to preserve the ordering such that existing colormaps do not move locations in the new generated
__init__.pyfile? I want to ensure that no existing colormap values have changed (unless they somehow had an actual error), but it's not feasible to diff that file because the colormaps are now in a different order. - I've made notes about various colormaps that now have multiple short names. I guess the idea here is to match the names provided upstream, but I don't think it's useful to do that unless again there was something wrong with the original short name (e.g. that it matches the name of another different colormap from elsewhere, like
blues). Barring such a reason, I'd prefer to keep it down to one long name and one short name per colormap, wherever possible.
| @@ -0,0 +1,100 @@ | |||
| # CET_merge.py - create updated `aliases` and `mapping` for CET_to_py.py | |||
There was a problem hiding this comment.
Can you explain a bit more at the top of this file, with something like "This script should be run when incorporating upstream changes to colormaps, allowing additional colormaps provided at XXXX to be added safely to colorcet without disrupting existing colormaps."
There was a problem hiding this comment.
And then I guess refer to README_assets.md, which documents the whole process.
There was a problem hiding this comment.
Sure, I can do that.
| diverging_gwr_55_95_c38 = ['gwr'], | ||
| diverging_gwv_55_95_c39 = ['gwv'], | ||
| diverging_linear_bjr_30_55_c53 = ['bjr'], | ||
| diverging_linear_bjy_30_90_c45 = ['bjy', 'divbjy'], |
There was a problem hiding this comment.
What's the reason for the extra alias divbjy?
There was a problem hiding this comment.
So all aliases are automatically pulled from colorcet.m (I didn't add any aliases on my own). colorcet.m links the following names: { 'D7' 'D07' 'BJY' 'DIVBJY' } to the descriptorname and map diverging-linear_bjy_30-90_c45_n256.
| linear_bgy_10_95_c74 = ['bgy'], | ||
| linear_bgyw_15_100_c68 = ['bgyw'], | ||
| linear_blue_5_95_c73 = ['kbc', 'linear_kbc_5_95_c73'], | ||
| linear_blue_95_50_c20 = ['depth', 'blues'], # mpl |
There was a problem hiding this comment.
Here I guess depth is preferred as a name over blues because there are lots of different blues colormaps around? Makes sense, though I don't know why it would be called depth in particular.
There was a problem hiding this comment.
Again, in colorcet.m, L12, and DEPTH are aliases for linear_blue_95-50_c20_n256, which were found with make_csvs_from_colorcet.m and merged in CET_merge.py. I guess it also happens to be the same as blues from matplotlib?
| linear_gow_65_90_c35 = ['geographic2'], | ||
| linear_green_5_95_c69 = ['kgy', 'linear_kgy_5_95_c69'], | ||
| linear_grey_0_100_c0 = ['grey', 'gray'], # mpl | ||
| linear_grey_10_95_c0 = ['reducedgrey', 'dimgray'], |
There was a problem hiding this comment.
Not sure what 'reducedgrey' adds as an alias here; seems like that will just make the space of names less usable for e.g. GUI colormap selector widgets, with several indistinguishable options.
There was a problem hiding this comment.
Same comment as above. reducedgrey is automatically imported from colorcet.m.
| linear_kbgoy_20_95_c57 = ['gouldian'], | ||
| linear_kbgyw_5_98_c62 = ['kbgyw'], | ||
| linear_kry_0_97_c73 = ['kry', 'yellowheat'], | ||
| linear_kryw_0_100_c71 = ['kryw', 'heat', 'fire'], |
There was a problem hiding this comment.
See previous comments.
|
If preserving the ordering requires miracles, then I guess what's sufficient is a script we can paste into this PR with its output, where the script imports the new version here and the latest released version of colorcet and reports whether any floating-point values differ for any previously defined colormap. |
These scripts and this PR do not modify or replace any existing colormaps. Only new maps were added, as well as some new aliases for existing maps that were found in That said, I think I could probably modify EDIT: The original order is dependent on the order returned by |
|
I could re-work this so only one short name is kept per map. Come to think of it, one thing that isn't ideal about the existing implementation the varying filenames for colormap CSV files. They really should all be named with the "descriptor name", e.g. Again, I'll think about it and get back to you. |
Agreed. That's the full, canonical name, as far as colorcet is concerned, even if upstream has shifted to a different naming style.
Right; just need to demonstrate that equality here in this PR before merging, either by preserving the original ordering (in which case git will be able to diff
To run that file it should be sufficient to do BTW, studying Overall, I'm not sure it's helpful to include the CET_L3 style names as aliases; they are neither canonical in terms of the underlying algorithm by which they were generated (as names like |
|
Oi, this has gotten complicated. Ok, I think I want to go back to scratch in another PR. I think my approach here has muddied things too much. Here's the new approach I propose:
|
Whatever you think is best, since you're doing the work! :-)
That makes sense, and should probably have been done for v2 originally. Alas!
Ok
I think that's an argument for documenting that mapping online, so that colorcet users can crossreference with Peter's work. But I'm concerned with the cost to what I think is a larger group of users who just want to use these in Python and aren't concerned with how the maps appear in other docs or in other languages. I'd favor keeping things simple for the casual Python users, while documenting the mapping somewhere (e.g. in a lookup dictionary we publish that translates the CET-* name to the underlying algorithmic name) rather than making the CET names appear everywhere. |
This is a second attempt at #62.
aliasesinCET_to_py.pyto a dict of lists. This rendersaliases_v2unnecessary. Minor changes necessary to makeCET_to_py.pyand thencolorcet/__init__.pywork with a dict of lists.make_csvs_from_colorcet.mandCET_merge.py, along with instructions inREADME_assets.md. The scripts were used to generate new CSVs for new colormaps and updateCET_to_py.pywhich in turn were used to updatecolorcet/__init__.pywith the new colormaps and aliases.aliasesin CET_to_py.py is now a dict of lists. This renders thealiases_v2dict unnecessary, as those can be added in toaliasesas additional values.CET_merge.pybut I think I finally got it right.assets/CETperceptual_csv_0_1_v3. It looked like for each existing cyclical colormap there was also one with 0.25 shift so I followed suit and included similarly-shifted maps along with the non-shifted maps (e.g. CET-C7s etc.)examples/assets/images/named.pngneeds to be updated, but I couldn't getexamples/assets/write_named.pyto run right on my machine. I got these warnings and the maps didn't plot right at all:get_aliases()a bit to just keep trying to get more aliases until no more are found. I hope the result is OK in terms of the order of results returned. I reworkedtest_get_aliases()to use sets and therefore not care about the order of the maps returned byget_aliases().CET_merge.pyshowing old aliases and mappings retained that are different fromcolorcet.m: