Closed randallpittman closed 3 years ago
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.
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.
@jbednar:
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 colorcet.m
.
That said, I think I could probably modify CET_merge.py
to preserve the previous order. I'll give it a bit of time and get back to you.
EDIT: The original order is dependent on the order returned by os.listdir
, as the maps are put into __init__.py
by traversing through the folders of CSVs. That said, I think I can hack a workaround to preserve the existing order.
@jbednar
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. linear_kryw_0-100_c71
, as that is the full identity of the colormap.
Again, I'll think about it and get back to you.
The CSV files really should all be named with the "descriptor name", e.g. linear_kryw_0-100_c71
Agreed. That's the full, canonical name, as far as colorcet is concerned, even if upstream has shifted to a different naming style.
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 colorcet.m.
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 __init__.py
) or by separately pasting an example of comparing before and after numerically.
I think examples/assets/images/named.png needs to be updated, but I couldn't get examples/assets/write_named.py it to run right on my machine. I got these warnings and the maps didn't plot right at all:
To run that file it should be sufficient to do conda install -c pyviz holoviews bokeh selenium ; conda install -c conda-forge firefox geckodriver
. It did need updates to match a recent change in Panel, so I added linked_axes=False
and balanced the columns of output (since their lengths differed now) and pushed the revised script and its current PNG output to your PR. If the colormap names change, it will still need updating, but I can do that after this PR is merged if it's difficult for you to run the script locally.
BTW, studying named.png
, rainbow2
seems visually identical to rainbow
, though I can see from the parameters it differs slightly. Unless there is a very strong reason to retain it I'd remove the short name to avoid confusion; people will wonder why there are two nearly identical colormaps for them to choose from. rainbow3
and rainbow4
are clearly distinct, and thus deserve short names.
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 linear_kryw_0_100_c71
are) nor are they easy to remember and distinguish (as names like "fire" are). So I continue to prefer publishing only one short name per colormap except in some special well-justified cases, and that short name should (in the absence of a strong reason otherwise) should be the name the colormap has always had in colorcet.
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:
git mv
" renamed to their algorithmic name and merged into a single folder. I made a script that counted 37 of the CET-*.csv files being redundant with the "v1" CSV files. After this, mapping
in CET_to_py.py
is no longer necessary.make_csvs_from_colorcet.m
to just generate new CSVs for colormaps we don't already have. These will be named with the algorithmic name found in colorcet.m
.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! :-)
All CETperceptual CSV files be "git mv" renamed to their algorithmic name and merged into a single folder. I made a script that counted 37 of the CET-*.csv files being redundant with the "v1" CSV files. After this, mapping in CET_to_py.py is no longer necessary.
That makes sense, and should probably have been done for v2 originally. Alas!
I'll rework make_csvs_from_colorcet.m to just generate new CSVs for colormaps we don't already have. These will be named with the algorithmic name found in colorcet.m.
Ok
Regarding aliases, I think there's an argument to be made for keeping the CET-* names, and that being that these are the shorthand names most used by Peter at colorcet.com in the gallery and user guide. Beyond those, however, I think it's fine to keep just one "colloquial" alias, and make sure it points to the same thing it has always pointed to.
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.
aliases
inCET_to_py.py
to a dict of lists. This rendersaliases_v2
unnecessary. Minor changes necessary to makeCET_to_py.py
and thencolorcet/__init__.py
work with a dict of lists.make_csvs_from_colorcet.m
andCET_merge.py
, along with instructions inREADME_assets.md
. The scripts were used to generate new CSVs for new colormaps and updateCET_to_py.py
which in turn were used to updatecolorcet/__init__.py
with the new colormaps and aliases.aliases
in CET_to_py.py is now a dict of lists. This renders thealiases_v2
dict unnecessary, as those can be added in toaliases
as additional values.CET_merge.py
but 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.png
needs to be updated, but I couldn't getexamples/assets/write_named.py
to run right on my machine. I got these warnings and the maps didn't plot right at all:It should be stated that I'm not a user of holoviews or bokeh, so I might have something set up wrong.
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.py
showing old aliases and mappings retained that are different fromcolorcet.m
: