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

Unstructured plots #1225

Merged
merged 11 commits into from Apr 24, 2015

Conversation

Projects
None yet
4 participants
@doutriaux1
Member

doutriaux1 commented Apr 20, 2015

doutriaux1 added some commits Apr 17, 2015

added test to test suite
make sure unstrucutred are not plotted against non valid methods
ctests pass again wasn't restrictive enoguh on grid checking
removed print in cdms2
added test case to make sure failing gms do fail

@doutriaux1 doutriaux1 referenced this pull request Apr 20, 2015

Merged

new baseline #35

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Apr 20, 2015

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Apr 20, 2015

@ThomasMaxwell can you please try it too? I'm a bit worried it breaks your unstructured grids! Your tests pass but I would feel better if you would try your unstructured grids plotting on this.

@ThomasMaxwell

This comment has been minimized.

Contributor

ThomasMaxwell commented Apr 20, 2015

I don’t see any effect of this branch upon the vcs3D unstructured grid plots.

— Tom

From: Charles Doutriaux <notifications@github.commailto:notifications@github.com>
Reply-To: UV-CDAT/uvcdat <reply@reply.github.commailto:reply@reply.github.com>
Date: Monday, April 20, 2015 at 2:47 PM
To: UV-CDAT/uvcdat <uvcdat@noreply.github.commailto:uvcdat@noreply.github.com>
Cc: "Maxwell, Thomas P. (GSFC-606.2)[SCIENCE APPLICATIONS INTL CORP]" <thomas.maxwell@nasa.govmailto:thomas.maxwell@nasa.gov>
Subject: Re: [uvcdat] Unstructured plots (#1225)

@ThomasMaxwellhttps://github.com/ThomasMaxwell can you please try it too? I'm a bit worried it breaks your unstructured grids! Your tests pass but I would feel better if you would try your unstructured grids plotting on this.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1225#issuecomment-94537605.

# Ok let's check for meshfill needed
if g0 is not None and (arglist[0] is not None and isinstance(arglist[0],cdms2.avariable.AbstractVariable) and not isinstance(arglist[0].getGrid(),cdms2.grid.AbstractRectGrid)) and arglist[3] not in ["meshfill",]:
raise RuntimeError("You are attempting to plot unstructured grid with a method that is not meshfill")

This comment has been minimized.

@doutriaux1

doutriaux1 Apr 20, 2015

Member

@ThomasMaxwell thanks for checking. Specifically that is the area I was worried about. Glad it didn't break your stuff.

@@ -2708,9 +2710,12 @@ def __plot (self, arglist, keyargs):
if float(doratio[:-1])==0.: doratio='0'
## Check for curvilinear grids, and wrap options !
if arglist[0] is not None:
g0=arglist[0].getGrid()

This comment has been minimized.

@aashish24

aashish24 Apr 23, 2015

Contributor

what is g0 represents here (why 0?) Can we use more descritive variable names?

@aashish24 aashish24 added this to the 2.2 milestone Apr 23, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 23, 2015

@dlonie can you approve this branch (just verify my changes). The tests are passing for me.

@doutriaux1 I cannot verify if the baseline looks correct. Can you double check on that please?

x.drawlogooff()
x.setbgoutputdimensions(1200,1091,units="pixels")
import vcs

This comment has been minimized.

@allisonvacanti

allisonvacanti Apr 23, 2015

Contributor

These imports (and the checkimage/open below) don't need to be repeated. Accidental copy/paste?

ret = checkimage.check_result_image(fnm,src,checkimage.defaultThreshold)
sys.exit(ret)
fnm = "test_plot_unstructured_via_boxfill.png"

This comment has been minimized.

@allisonvacanti

allisonvacanti Apr 23, 2015

Contributor

Same here, is this repeated intentionally?

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented on Packages/vcs/Lib/Canvas.py in d6b81af Apr 23, 2015

237 characters O.o

This comment has been minimized.

Contributor

aashish24 replied Apr 23, 2015

Okay, I will wrap it as well 😄

This comment has been minimized.

Contributor

aashish24 replied Apr 23, 2015

ahh..too many style issues. I will push a new PR for that.

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Apr 23, 2015

I don't see any functional issues, but the repeated lines of code in the test are confusing.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 23, 2015

good catch. I took care of it. Please have a look again

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented on testing/vcs/test_plot_unstructured_via_boxfill.py in 3d1a738 Apr 23, 2015

From here to x.plot(...) (lines 16-27 in the new diff) looks repeated as well.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 23, 2015

Grr...Okay..pushed a fix again.

@aashish24 aashish24 force-pushed the unstructured_plots branch from 7c52dac to 2d79cba Apr 23, 2015

@aashish24 aashish24 force-pushed the unstructured_plots branch from 2d79cba to d7bf1c3 Apr 23, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 23, 2015

@dlonie I guess it should be all good now.

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Apr 23, 2015

+1. You beat me to the missing newline ;)

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Apr 23, 2015

Can't merge for conflicts, though.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 23, 2015

@dlonie wait before you review it.

@aashish24 aashish24 force-pushed the unstructured_plots branch from f51e761 to 45db658 Apr 23, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 23, 2015

Okay now it should be good to review.

@@ -266,6 +266,15 @@ cdat_add_test(vcs_test_taylor_2_quads
# These test actually plot things need sample data
if (CDAT_DOWNLOAD_SAMPLE_DATA)
cdat_add_test(test_vcs_plot_unstructured_via_non_meshfill_or_default_fails

This comment has been minimized.

@allisonvacanti

allisonvacanti Apr 23, 2015

Contributor

This file seems to be missing from the branch.

This comment has been minimized.

@allisonvacanti

allisonvacanti Apr 23, 2015

Contributor

The test script, that is.

@@ -112,8 +112,8 @@
"datawc_x2": 1e+20,
"datawc_y1": 1e+20,
"datawc_y2": 1e+20,
"ext_1": false,
"ext_2": false,
"ext_1": false,

This comment has been minimized.

@allisonvacanti

allisonvacanti Apr 23, 2015

Contributor

This is breaking the vcs_test_dump_json test:

test 62
    Start 62: vcs_test_dump_json

62: Test command: /ssd/src/uvcdat-build/install/bin/python "/ssd/src/uvcdat/testing/vcs/test_dump_json.py" "/ssd/src/uvcdat/testing/vcs/test_vcs_dump_json.json"
62: Test timeout computed to be: 1500
62: Traceback (most recent call last):
62:   File "/ssd/src/uvcdat/testing/vcs/test_dump_json.py", line 31, in <module>
62:     assert(filecmp.cmp("test_vcs_dump_json.json",src))
62: AssertionError
1/1 Test #62: vcs_test_dump_json ...............***Failed    1.05 sec
@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 23, 2015

Ah, okay, I don't know why I didn't see that in my ctest. Let me have a look at it.

aashish24 added some commits Apr 23, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 23, 2015

@dlonie Should be fixed now.

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Apr 24, 2015

Tests pass, change looks good \o/

allisonvacanti added a commit that referenced this pull request Apr 24, 2015

@allisonvacanti allisonvacanti merged commit 117d997 into master Apr 24, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@doutriaux1 doutriaux1 deleted the unstructured_plots branch May 14, 2015

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