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

Replace colormaps with python-colorspace palettes #587

Merged
merged 5 commits into from Nov 26, 2018

Conversation

Projects
None yet
4 participants
@sadielbartholomew
Copy link
Contributor

sadielbartholomew commented Oct 31, 2018

To close #548.

Currently a 'proof of concept' waiting on agreement that the palette choices are the best available in context. The python-colorspace package provides a selection of default palettes & I made a choice for the three colour sets used by OGGM (for altitude, section thickness & glacier thickness) based on the considerations below, however before I continue, I would like to ensure there is agreement on the choices made (which are trivial to change by editing the palette name strings in the code).

  • Thickness & altitude are continuous, where (I believe, though I am not a glaciologist so correct me if I am wrong) extremes rather than central values would be of most interest, so a choice should come from the 'sequential' palettes and not the 'diverging' or 'qualitative'.
  • Colour sets used should not clash with the greyscale of the background terrain image, nor with the 'rainbow' colours of the plotted flowline. In particular, this means palettes with yellow should be chosen over those with white for one side of the scale.
  • Altitude colouring should subtle, as it is subsidary information, but the thickness colour sets should stand out as much as possible.
  • The two thickness colours should easy to distinguish from each other. In fact, ideally the three palettes would be distinct to allow for immediate comprehension of the depicted data.
  • Glaciers physically are 'cool blue' in colour, so it would be nice in one case to use a palette close to the reality!

Therefore, I went for "Greens 2" for altitude (like grass, with white 'snow' at higher altitudes), "Blue-Yellow" (glacier-like colours) for section thickness & "Heat" for glacier thickness (red-based to make it clearly different to the blues of section thickness). Example images (compare to those in OGGM/oggm-sample-data/baseline_images/freetype_28 i.e. topmost, central & last):

new_test_chhota_shigri
new_test_downstream
new_test_thick_alt

Let me know your thoughts! There is no point adapting the documented & test baseline images until you are happy with the choices.

To do:

  • Apply code to get HCL color palettes working (images correct by eyeball except for colour changes, & pytest oggm/tests/test_graphics.py passes without --mpl option).
  • Add new dependencies to the OGGM installation environment.
  • Document the change & new dependency (& contributor).
  • Agree palette choices [input required].
  • Change test baseline images to account for new colour sets so pytest --mpl oggm/tests/test_graphics.py passes (i.e. update test images to the new standards).[with python-colorspace as an optional dependency this is no longer required]
  • Update documentation images to account for new colour sets. [as above]
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Oct 31, 2018

Hello @sadielbartholomew! Thanks for updating the PR.

Comment last updated on October 31, 2018 at 15:31 Hours UTC

@sadielbartholomew sadielbartholomew changed the title Change colormaps to HCL-based python-colorspace palettes Change colormaps to python-colorspace HCL palettes Oct 31, 2018

@sadielbartholomew sadielbartholomew changed the title Change colormaps to python-colorspace HCL palettes Replace colormaps with python-colorspace palettes Oct 31, 2018

@sadielbartholomew sadielbartholomew force-pushed the sadielbartholomew:i548-hcl-colorspace branch from f39268a to ba162ba Oct 31, 2018

@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Nov 1, 2018

Thanks a lot! This is looking good ;-)

This will need some discussion indeed but I'm a bit on a rush now so I'll try to get back to you next week!

@sadielbartholomew

This comment has been minimized.

Copy link
Contributor Author

sadielbartholomew commented Nov 1, 2018

Of course @fmaussion, please take your time. I realise it is a low-importance change, so I do not expect quick responses. Thanks.

@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Nov 16, 2018

Sorry it took me so long to get back to you. I hope you still have some time to spend on this! I like the new colors! Thanks a lot

My few comments:

  • I like the use of global module variables to define the colormaps - especially because:
  • I would prefer not to make of python-colorspace a hard dependency of OGGM (yet) - i.e. the global variables should default to the previous cmaps when python-colorspace is not installed. This would also allow for not installing it on Travis and let the graphics tests pass for now.

Now to the colors:

  • I like the blue-yellow colormap for ice thickness most, and I would use it for both the glacier and section thickness. However, I feel it should be inverted (blue for thicker areas)
  • I can't help it: I understand the need for HCL cmaps for everything but topography. See for example:

img

As opposed to the one with "Greens" above. Maybe @retostauffer wants to chime in and let us know his suggestion for colors and topography ;)

@sadielbartholomew

This comment has been minimized.

Copy link
Contributor Author

sadielbartholomew commented Nov 21, 2018

Thanks for your detailed feedback @fmaussion!

Sorry it took me so long to get back to you. I hope you still have some time to spend on this!

No problem, I appreciate you fitting in time to look at this. I would like to finish this PR, though I am approaching a busier period now so I will dedicate some time this week to update it according to your feedback, but if I can't complete it so you are happy by then I will have to wait until mid-December to have another look. You can always take over this if you want to accelerate the change.

In hindsight, the terrain is certainly too subtle on my original proposal. I will revert the topography colormap. I will also invert the blue-yellow colormap as you suggest, & apply that same scheme for the glacier thickness plots. And of course implement the main requirement, to check if python-colorspace is installed & if not just apply the current colormaps. I presume a try ... except ImportError statement would be the way to do this, though I will need to check it works & work out where best to place it.

Please feel free to comment further if you have any changes of mind for the colourmaps (or approach) still, as there are plenty to choose from & it is trivial to amend the selections. Otherwise I will update you when this PR is ready for re-review. Thanks again.

sadielbartholomew added some commits Nov 24, 2018

@sadielbartholomew sadielbartholomew force-pushed the sadielbartholomew:i548-hcl-colorspace branch from 16c3d96 to b95653c Nov 24, 2018

@sadielbartholomew

This comment has been minimized.

Copy link
Contributor Author

sadielbartholomew commented Nov 24, 2018

As well as the changes discussed above, I have upped the CMAP_DIVS integer in graphics.py to create a smoother mapping in line with the current (master branch) colormaps.

Sample images (same examples as in my initial comment) with the new colour set choice specifications (the altitude colormap is now back to as in the main codebase, so the central image from my initial comment is identical to the original reference linked there):

test_chhota_new1
test_thick_alt_new1

The pale yellow hue for small section thickness isn't the most distinct against the white/grey of the background terrain image, but as long as this does not concern you I think these palette choices work well. If you prefer, however, I could crop the lower end of the color palette so that the scale zero is mapped to a slightly greener tinge?

setup.py Outdated
@@ -63,7 +63,8 @@
'progressbar2',
'boto3',
'requests',
'salem']
'salem',
'python-colorspace']

This comment has been minimized.

Copy link
@fmaussion

fmaussion Nov 24, 2018

Member

Can you also remove it from setup.py?

This comment has been minimized.

Copy link
@sadielbartholomew

sadielbartholomew Nov 24, 2018

Author Contributor

I'm just doing that now & about to squash it in (sorry, I didn't think anyone would be online at the present time).

@fmaussion
Copy link
Member

fmaussion left a comment

Pending the remaining comment in setup.py this looks good to me? I find the new colormaps are great like this! Thanks a lot!

@sadielbartholomew sadielbartholomew force-pushed the sadielbartholomew:i548-hcl-colorspace branch from b95653c to bb8ffb1 Nov 24, 2018

@sadielbartholomew sadielbartholomew force-pushed the sadielbartholomew:i548-hcl-colorspace branch from bb8ffb1 to f59177b Nov 24, 2018

@sadielbartholomew

This comment has been minimized.

Copy link
Contributor Author

sadielbartholomew commented Nov 24, 2018

Glad to help @fmaussion. Thanks for the rapid feedback.

@sadielbartholomew

This comment has been minimized.

Copy link
Contributor Author

sadielbartholomew commented Nov 24, 2018

(One Travis CI job fails but it is unrelated I believe. Code coverage decreases a little, naturally, but I think it would be overkill at this stage, i.e. for this PR, to create image comparison tests.)

@TimoRoth

This comment has been minimized.

Copy link
Contributor

TimoRoth commented Nov 24, 2018

Docker Hub seems to be having issues the last couple days. Builds are randomly failing left and right on various services because of it.

@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Nov 26, 2018

Thanks!

@fmaussion fmaussion merged commit c199e03 into OGGM:master Nov 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 86.219%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.