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

Colormaps #1693

Merged
merged 26 commits into from Nov 18, 2015
Merged

Colormaps #1693

merged 26 commits into from Nov 18, 2015

Conversation

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Nov 14, 2015

@aashish24 @sankhesh I'm still teaking a few things, but somehow when merging master back in, meshfill animations test is now broken @sankhesh do you mind taking a look at this while I fine tune this PR? Thanks.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Nov 16, 2015

@aashish24 @chaosphere2112 @sankhesh this 20mn PR is finally ready for review. I still need help from @sankhesh for the meshfill animation which I think I broke when merging master in.
Basic idea of this PR: allow transparency on colors, allow to set the whole 256 colors on a vcs colormap, conversion tool from matplotlib cmaps to vcs ones and allow to set a color directly by its "name" or rgba value.
Goes with: CDAT/uvcdat-testdata#84

@sankhesh
Copy link
Contributor

@sankhesh sankhesh commented Nov 16, 2015

@doutriaux1 I'll look into it today. Are we allowed to commit changes today?

@chaosphere2112
Copy link
Contributor

@chaosphere2112 chaosphere2112 commented Nov 16, 2015

@sankhesh Yeah, email out from @aashish24 on Friday indicated freeze was 11:59PM tonight.

@jbeezley
Copy link

@jbeezley jbeezley commented Nov 16, 2015

Yes, but which timezone? 😜

@chaosphere2112
Copy link
Contributor

@chaosphere2112 chaosphere2112 commented Nov 16, 2015

@jbeezley Let's shoot for UTC-11 😉

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 16, 2015

Yes, but which timezone?

In the email I referred to 11:59 PM PT

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 16, 2015

As per our last meeting, the code freeze will happen on Monday night 11:59 PT. Please try to look at the bugs assigned to you for release 2.4. If for some reason, you think you cannot get to resolve them in time, please contact Dean, Charles, or myself.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 16, 2015

its common to have 20mn PR become 2hr PR 😃

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Nov 16, 2015

I know but this one went into a 2 weeks PR...

@@ -231,12 +288,12 @@ def __init__(self, Cp_name, Cp_name_src='default'):
# Note: See RGB_Table Class for "index" setting of the colormap entries

# Set a colorcell RGB
def setcolorcell(self, index, red, green, blue):
def setcolorcell(self, index, red, green, blue, alpha=100.):
Copy link
Contributor

@aashish24 aashish24 Nov 16, 2015

why default to 100?

Copy link
Contributor

@chaosphere2112 chaosphere2112 Nov 16, 2015

Legacy support. All of the old usages of this function would have generated opaque colors, no reason to change that behavior.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 16, 2015

@doutriaux1 at a high level, the changes makes sense to me. Only had one question but that's because of me not knowing certain things.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 16, 2015

LGTM 👍 once we fix the conflict

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 16, 2015

@doutriaux1 is going to fix the merge conflict..

aashish24
Copy link
Contributor

aashish24 commented on 615e5fa Nov 18, 2015

@doutriaux1 interesting but I am wondering how this code break the animation...

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@doutriaux1 new changes looks reasonable to me. I am going to wait for the buildbot.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@doutriaux1 @sankhesh Looks like the test is still failing (animated meshfill). What I am missing?

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@doutriaux1 @sankhesh I figured it out. Atleast one of the problem is missing baseline. I am adding it now

For testing, we need background (no window) to be true.
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@sankhesh @doutriaux1 @williams13 I fixed the animation test (really tiny fix). Not sure about the pattern failing test. @sankhesh can you look into it?

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Nov 18, 2015

@aashish24 that's why when I looked at the meshfill test bg was set to 1 (should have been True really but i will let it fly unless I need to trigger a new build). I need to push the mercator baseline which is why the test fails. Will fix tomorrow. In the mean time when you get in @aashish24 do you mind merging this branch into @sankhesh pending branches so that we can approve them all quickly when I get in tomorrow? Thanks

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Nov 18, 2015

mac seem to be broken again, I wonder if the lab scans repatched them again....

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@doutriaux1 test patterns is also failing. Earlier 4 tests were failing but now only three are failing on garant. Do you still want me to merge it?

self,
'fillareacolors',
value)
self._fillareacolors = value
Copy link
Contributor

@sankhesh sankhesh Nov 18, 2015

We allow the user to set a null value? Is that intentional?

Copy link
Contributor Author

@doutriaux1 doutriaux1 Nov 18, 2015

Yes, I think for missing value color None used to mean transparent

Copy link
Contributor

@sankhesh sankhesh Nov 18, 2015

Okay. But that shouldn't be the case anymore, right? Since we have an opacity field now.

@sankhesh
Copy link
Contributor

@sankhesh sankhesh commented Nov 18, 2015

@doutriaux1 👍 LGTM

I'm looking at merging my two other branches.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@sankhesh do you think the tests failing are not because of this branch?

@sankhesh
Copy link
Contributor

@sankhesh sankhesh commented Nov 18, 2015

@aashish24

The tests are definitely failing because of this branch. But the failures require updating the baselines.

vcs_test_click_info is a weird one. Fails on some machines only.

@sankhesh sankhesh mentioned this pull request Nov 18, 2015
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Nov 18, 2015

@sankhesh I think vcs_test_click_info and a few others are screen size dependent, @chaosphere2112 was going to look into this. BTW @chaosphere2112 you can now do a x.open(800,600) to get the correct window size.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@doutriaux1 new baseline?

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Nov 18, 2015

@aashish24 it should all be ready now, let's wait for the bots.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Nov 18, 2015

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Nov 18, 2015

@aashish24 I tihnk it's ready to merge.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Nov 18, 2015

@aashish24 crunchy (RH6 FULL) died so it will never complete

@chaosphere2112
Copy link
Contributor

@chaosphere2112 chaosphere2112 commented Nov 18, 2015

@doutriaux1 @aashish24 Tests all pass for me and the bots look good; I'm merging.

chaosphere2112 added a commit that referenced this issue Nov 18, 2015
@chaosphere2112 chaosphere2112 merged commit 02b035f into master Nov 18, 2015
6 of 9 checks passed
@doutriaux1 doutriaux1 deleted the colormaps branch Nov 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants