-
Notifications
You must be signed in to change notification settings - Fork 68
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
Unstructured plots #1225
Conversation
make sure unstrucutred are not plotted against non valid methods
removed print in cdms2 added test case to make sure failing gms do fail
goes with: CDAT/uvcdat-testdata#35 |
@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. |
I don’t see any effect of this branch upon the vcs3D unstructured grid plots. — Tom From: Charles Doutriaux <[email protected]mailto:[email protected]> @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. — |
|
||
# 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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasMaxwell thanks for checking. Specifically that is the area I was worried about. Glad it didn't break your stuff.
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These imports (and the checkimage/open below) don't need to be repeated. Accidental copy/paste?
I don't see any functional issues, but the repeated lines of code in the test are confusing. |
good catch. I took care of it. Please have a look again |
Grr...Okay..pushed a fix again. |
7c52dac
to
2d79cba
Compare
2d79cba
to
d7bf1c3
Compare
@dlonie I guess it should be all good now. |
+1. You beat me to the missing newline ;) |
Can't merge for conflicts, though. |
@dlonie wait before you review it. |
Conflicts: testing/vcs/CMakeLists.txt
f51e761
to
45db658
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems to be missing from the branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test script, that is.
Ah, okay, I don't know why I didn't see that in my ctest. Let me have a look at it. |
@dlonie Should be fixed now. |
Tests pass, change looks good \o/ |
@dlonie @chaosphere2112 please review
dashboard: https://open.cdash.org/buildSummary.php?buildid=3780331