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

Continents & Continent Lines #1678

Merged
merged 8 commits into from Nov 17, 2015

Conversation

Projects
None yet
4 participants
@chaosphere2112
Contributor

chaosphere2112 commented Nov 12, 2015

This PR fixes some existing functionality (changing what continents to use) and adds a new piece of functionality (changing the appearance of the lines used to draw the continents).

While we technically supported drawing continents of a variety of types using a keyword argument (continents=1-12) on plot, as soon as a call to configureEvent on the VTK Backend happened, the continents were reset to the canvas' globally defined setting.

Now, we can do things like this plot:

nice_continents

@doutriaux1 @aashish24

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Nov 12, 2015

@chaosphere2112 some failures

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 13, 2015

Great.. @chaosphere2112 what issue this fixes it? Did we have existing issue on github for it?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 13, 2015

Is it going to conflict with #1688? @sankhesh @chaosphere2112?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 13, 2015

@chaosphere2112 @sankhesh yes, I think there is going to some conflict (or code duplication), if I am not mistaken. If that's the case, please coordinate with me or @doutriaux1 before hand.

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Nov 13, 2015

I don't see a conflict with #1688 since most of my changes were for isoline pipeline only.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 13, 2015

@aashish24 Nope, no conflicts. This is a completely separate piece of code.
@doutriaux1 Whoops, looks like I neglected to call a validation function, just the module. Running the tests locally again and sending up.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 13, 2015

Cool! thanks @chaosphere2112 @sankhesh there seems to be some code duplication on line stipple pattern..

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 13, 2015

@aashish24 There technically already is the code duplication (the code you see in my PR was just refactored out of prepLine). If we merge mine, then we can just use the same stippleLine function in the isolines.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 13, 2015

So found another bug while I was poking around; had an extraneous int() cast that was making all of the continents black (whoops!), which was left over from before I fixed colors properly (I thought they were supposed to be in the range 0-255, but they needed to be 0.-1.). Also updated the baseline to match the correct color output (the baseline was made while I was doing 0-255, which meant it was SUPER RED AND MAYBE SOME OTHER COLORS rather than kind of dark red).

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 13, 2015

sure @chaosphere2112 sounds great@ thanks!

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Nov 13, 2015

Sounds good. I can make the change to #1688 once this is merged.

@sankhesh sankhesh referenced this pull request Nov 16, 2015

Merged

1625 line stipple #1688

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 16, 2015

@chaosphere2112 continent lines test is failing..

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 16, 2015

@aashish24 Looks like it's a threshold issue. The diffs are almost impossible to see.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 16, 2015

@aashish24 I'll update the threshold.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 16, 2015

@aashish24 and fix the merge conflict while I'm at it

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 17, 2015

thanks @chaosphere2112 now it looks like multiple tests are failing. May be related to a bad conflict fix?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 17, 2015

@chaosphere2112 the lines were thicker in test than in validated image. For @doutriaux1 and me it was obvious but may be we missed something?

aashish24 and others added some commits Nov 17, 2015

@aashish24 aashish24 force-pushed the continents_lines branch from a7e4040 to c74dc12 Nov 17, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 17, 2015

@chaosphere2112 I rebased this branch again with a hope that it fixes the failures I have in the last rebase. Sorry, I couldn't ask you to do it because of the time of the day but I cherry-picked your threshold change.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 17, 2015

@doutriaux1 the branch is looking good on my machine and on other linux dashboards. Macs are not building because of NUMPY. I am going to merge this one soon if I don't hear any objection from you.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 17, 2015

LGTM 👍

aashish24 added a commit that referenced this pull request Nov 17, 2015

Merge pull request #1678 from UV-CDAT/continents_lines
Continents & Continent Lines

@aashish24 aashish24 merged commit f6db0b6 into master Nov 17, 2015

2 of 9 checks passed

cont-int/LLNL/Darwin-Mac 10.10.5 (LEAN) running 'make -j4' (Mon Nov 16 20:24:43 2015)
Details
cont-int/LLNL/Darwin-Mac2 10.10.5 (FULL) running 'make -j4' (Mon Nov 16 20:30:17 2015)
Details
cont-int/LLNL/Linux-oceanonly RH6 (MESA/NOGUI) running 'ctest -j12 -D Experimental' (Mon Nov 16 20:42:59 2015)
Details
continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build done.
Details
cont-int/LLNL/Darwin-Mac1 10.10.5 (NOGUI) running 'In Queue: 1' (Mon Nov 16 20:16:12 2015)
Details
cont-int/LLNL/Linux-annie Ubuntu 15.04 (FULL/MESA) running 'In Queue: 1' (Mon Nov 16 20:16:14 2015)
Details
cont-int/LLNL/Linux-crunchy RH6 (FULL) running 'In Queue: 2' (Mon Nov 16 20:16:13 2015)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aashish24 aashish24 deleted the continents_lines branch Nov 17, 2015

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Nov 17, 2015

@aashish24 thanks is there a baseline with this? did oyu merge it?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Nov 17, 2015

@aashish24 I don't see a baseline, the test added should have a new baseline. Did all the test passed for you?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Nov 17, 2015

@chaosphere2112 baseline?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 17, 2015

@doutriaux1 it passed on my system. test_continents.png seems to be in the baseline already

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