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

Proposed new color names #2065

Merged
merged 6 commits into from Aug 15, 2016

Conversation

Projects
None yet
6 participants
@aashish24
Contributor

aashish24 commented Jul 27, 2016

No description provided.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jul 27, 2016

@doutriaux1 @danlipsa I would like to propose refactoring the color names. The current color names unfortunately were inconsistent and hard to read. I followed the color names syntax as I found in some other tools specifically in R domain (https://www.nceas.ucsb.edu/~frazier/RSpatialGuides/colorPaletteCheatsheet.pdf) but I am open for suggestions. This in preparation for 3.0 release so, yes it will break the backward compatibility, but if you all insist, I can add a alias for now as well.

Also, over all the names got shorter, I deleted 277 characters and added 257.

Thoughts?

@aashish24 aashish24 added this to the 3.0 milestone Jul 27, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jul 27, 2016

@aashish24 yes, let's add an alias. Also let's add matplotlib palettes especially viridis because I want to switch to viridis for our default in a separate PR.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jul 27, 2016

@aashish24 I will add viridis and other matplotlib colormaps

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jul 27, 2016

@aashish24 I will add viridis and other matplotlib colormaps

@doutriaux1 if its okay, let me add that (and alias) so that we can have one branch for color maps.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jul 27, 2016

thanks @doutriaux1 so sounds like these changes are liked which is great. I think it will really help boost vcs usability as we all are making the API better

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jul 27, 2016

i was going to do it in that branch but go ahead. Thanks!

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jul 27, 2016

i was going to do it in that branch but go ahead. Thanks!

roger that

@aashish24 aashish24 force-pushed the vcs_color_names branch 2 times, most recently from b23aa8b to 2512eb0 Aug 4, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 5, 2016

@doutriaux1 @danlipsa this should be ready for review

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 5, 2016

Also, added matplotlib colors (plasma, inferno, magma, viridis)

@@ -4961,6 +4979,15 @@ def setcolormap(self, name):
# updateVCSsegments_flag = args[1]
# except:
# updateVCSsegments_flag = 1
# Python < 3 DeprecationWarning ignored by default

This comment has been minimized.

@doutriaux1

doutriaux1 Aug 5, 2016

Member

@aashish24 I don't think this should be here. We should issue this warning whenever the user sets the colormap instead.

This comment has been minimized.

@aashish24

aashish24 Aug 5, 2016

Contributor

thanks. @doutriaux1 I am bit confused. Currently the warning is in "setcolormap" instead. Look at the code at line 4953.

This comment has been minimized.

@doutriaux1

doutriaux1 Aug 5, 2016

Member

graphic methods can have colormaps as well, is that going to be triggered if I do:

import vcs
b = vcs.createboxfill()
b.colormap = "white_to_red"

This comment has been minimized.

@doutriaux1

doutriaux1 Aug 5, 2016

Member

remember there are 3 levels to look for the colormap used.
1- graphics method
2- canvas
3 vcs module

This comment has been minimized.

@aashish24

aashish24 Aug 9, 2016

Contributor

remember there are 3 levels to look for the colormap used.
1- graphics method
2- canvas
3 vcs module

Okay, let me add something to the code to make sure that we can map old to new regardless of where we set the colormap.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 5, 2016

@aashish24 also the initial.attributes in Share is just ridiculously long, we should reformat it to a straight json dump.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 5, 2016

@aashish24 let's also use this PR to switch the default colormap to viridis do you want me to do this?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 5, 2016

@aashish24 also the initial.attributes in Share is just ridiculously long, we should reformat it to a straight json dump.

we could do that. I made a pass so that its pretty. Even though its long, its easy to read. I am fine either way.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 5, 2016

@aashish24 let's also use this PR to switch the default colormap to viridis do you want me to do this?

@doutriaux1 okay if we do it in a separate PR to keep pr's small?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 5, 2016

that's fine by me

@aashish24 aashish24 force-pushed the vcs_color_names branch from d9af7ac to a31dbc4 Aug 7, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 7, 2016

@doutriaux1 I uglified the initial attributes as suggested, that made the file size three times smaller, I liked your idea, thanks. This branch is ready for review now.

@aashish24 aashish24 force-pushed the vcs_color_names branch from 00c7a66 to 018e8d7 Aug 7, 2016

@aashish24 aashish24 force-pushed the vcs_color_names branch from 018e8d7 to 8e3fc58 Aug 7, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 9, 2016

@doutriaux1 @danlipsa @williams13 I am going to push one more change for backward compatibility and then this will be ready for review.

@cdatrobot

This comment has been minimized.

cdatrobot commented Aug 9, 2016

Basic content checks passed!

Branch-at: 8e3fc58
Acked-by: @llnlbot

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 9, 2016

@aashish24 The new names LGTM 👍

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Aug 9, 2016

Do: test

@cdatrobot

This comment has been minimized.

cdatrobot commented Aug 9, 2016

Testing commands handed to buildbot.

Branch-at: 8e3fc58

@kwrobot

This comment has been minimized.

Member

kwrobot commented Aug 9, 2016

Your merge request has been queued for testing. You may view the test results on CDash or Buildbot.

Branch-at: 8e3fc58

@aashish24 aashish24 force-pushed the vcs_color_names branch from a2ea896 to 89c3c52 Aug 9, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 9, 2016

@doutriaux1 @danlipsa okay so I updated the code so that it will be backward compatible no matter how we set the colormap. Please have a look at the code now. Its ready for the review 🃏

@cdatrobot

This comment has been minimized.

cdatrobot commented Aug 9, 2016

Basic content checks passed!

Changes since last check: compare

Branch-at: 89c3c52
Acked-by: @llnlbot

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 10, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 10, 2016

@aashish24 LGTM 👍 i would squash the commits though

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 10, 2016

Do: test

@cdatrobot

This comment has been minimized.

cdatrobot commented Aug 10, 2016

Testing commands handed to buildbot.

Branch-at: 89c3c52

@cdatrobot

This comment has been minimized.

cdatrobot commented Aug 10, 2016

Your merge request has been queued for testing. You may view the test results on CDash or Buildbot.

Branch-at: 89c3c52

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 10, 2016

@aashish24 LGTM i would squash the commits though

@danlipsa each commit is different and brings new stuff to the code base. I am not entirely sure that in this particular case squashing makes sense. Actually if you look the branch there are only 4 commits as I rebased so the older commits are gone if that's what you were referring to.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Aug 10, 2016

@aashish24 Why would you have 'Added backward compatibility support' and 'Updated backward compatibility' as commits by themselves? Also, why would you have 'Proposed new color names' and 'Updated to use new color names' as separate commits?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 11, 2016

@aashish24 does graphic_method.colormap = "old_name" issues the warning?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 11, 2016

@aashish24 Why would you have 'Added backward compatibility support' and 'Updated backward compatibility' as commits by themselves? Also, why would you have 'Proposed new color names' and 'Updated to use new color names' as separate commits?

the first commit added the backward compatibility in a separate way which is different than the last commit on the same topic. I kept old to keep that history of what things I have tried. The proposed color names just added new names and the updated to use new color names updated tests to use new names vs the old ones. I am fine with squashing this into one.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 11, 2016

@aashish24 does graphic_method.colormap = "old_name" issues the warning?

@doutriaux1 no it does not. I thought about it and I don't think with direct assignment like this we can issue a warning. Now we can issue a warning afterwards when we actually try to use the color name but it may not be as useful (especially since we user colormap at different places and very differently). Managing that could be lot of work. I think we document it and send an email to the mailing list that may suffice to inform the user but I am open for ideas.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 11, 2016

@aashish24 it should be a property so you can issue the warning in the setter function

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 11, 2016

@aashish24 it should be a property so you can issue the warning in the setter function

I see that but then I guess I have to create a override function for all graphics method (there is no common baseclass?) Its more core (canvas and all other graphics methods etc..) but i am fine with that if that is more useful.

@doutriaux1 doutriaux1 merged commit 2fcf573 into master Aug 15, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@doutriaux1 doutriaux1 deleted the vcs_color_names branch Aug 15, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Aug 16, 2016

thanks @doutriaux1. I am wondering if we should have a base class for graphics methods. Thoughts? @doutriaux1 @danlipsa

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Aug 16, 2016

I've been thinking about it for a while, there's something similar to that in vcsaddons, we should inspire ourselves from it.

chaosphere2112 pushed a commit that referenced this pull request Sep 1, 2016

Proposed new color names (#2065)
* Proposed new color names

* Added backward compatibility support

* Updated to use new color names

* Added perceptual sequential matplotlib colormap

* Updated backward compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment