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

BUG #1106: wrong dataset created from nc file with decreasing lat and… #1706

Merged
merged 4 commits into from Dec 2, 2015

Conversation

Projects
None yet
6 participants
@danlipsa
Contributor

danlipsa commented Nov 24, 2015

… bounds

We did not handle correctly a nc file which specifies bounds and
has decreasing latitude or longitude.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 24, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 24, 2015

@danlipsa this is great! I will try it later unless someone else beats me to it. Do we have a test?

BUG #1106: wrong dataset created from nc file with decreasing lat and…
… bounds

We did not handle correctly a nc file which specifies bounds and
has decreasing latitude or longitude.
@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 24, 2015

@aashish24 No, I did not add one. Should I add just the script/data that was used to show the bug?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 24, 2015

@doutriaux1 @durack1 There are some tests failing where the test image seems correct and baseline seems wrong. Could you confirm that, for instance by looking at old baselines or running the tests with a pre-vtk uvcdat? See the boxfill tests in https://open.cdash.org/viewTest.php?onlyfailed&buildid=4116767

@danlipsa danlipsa force-pushed the continents-off branch from 49d80ef to 658ac44 Nov 24, 2015

@durack1

This comment has been minimized.

Member

durack1 commented Nov 24, 2015

@danlipsa I see what you mean but I'm not certain what changes have occurred that could have caused this - maybe @chaosphere2112 can answer if @doutriaux1 is not currently available..? I wonder if some of the baselines need to be updated?

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Nov 24, 2015

I'm curious as to why the isofill tests are not failing.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 24, 2015

@danlipsa @sankhesh @durack1 I'm waiting on the internet to stop being terrible here, then I'll take a quick look

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 24, 2015

@danlipsa Looks like we still need to bump up the meshfill tests' threshold a bit more (they're failing on Annie; I'd say go to 40). As for the actual content of this PR, I tend to agree that the updated images look better. I'd want to wait on @doutriaux1 to comment when he gets back before merging this one, though.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 25, 2015

@danlipsa can you look into these failing tests? I see that box fills are failing as well

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 25, 2015

@aashish24 see the discussion earlier in the pull request. We believe the test images are correct and the baseline are wrong. We wait for @doutriaux1 to take a look at this as well.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 30, 2015

@doutriaux1 can you look at this branch and merge it if all looks good?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 1, 2015

will retrigger now and take a look tomororw morning

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Dec 1, 2015

@danlipsa if @doutriaux1 approves / agrees that new image (test image) is better than I would suggest you or @doutriaux1 update the baselines specifically for the NH ones..

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 1, 2015

@danlipsa building now.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 1, 2015

@danlipsa did you run this test on the seaice test? I think it should fix it. In the mean time I will create a baseline update. Also if it fixes the sea-ice could you add it as a test case? (put the nc file in uvcdat-testdata repo under data )

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Dec 1, 2015

@doutriaux1 yes, it fixes the sea ice test. @aashish24 also suggested adding a test for the sea ice and I meant to answer him: My thought is that we have all these other tests that exhibited the same faulty behavior - an empty area at the pole and all data shifted. Updating the baseline for these tests would be enough to test that we maintain the correct behavior. Adding another test for sea ice would not test anything new. What do you guys think?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 1, 2015

@danlipsa the other tests while exhibiting this issue were not meant to test this though, so I think it's worth a dedicated test.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 1, 2015

on that note @aashish24 the projection is still wrong....

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Dec 1, 2015

on that note @aashish24 the projection is still wrong....

@doutriaux1 are you talking about aeqd projection?

updated a few tests
png ones did not clean up after themselves
click_labels wasn't opening at proper size
box_custom wasn't retrning a failure code when failing
@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 1, 2015

updated baselines at: CDAT/uvcdat-testdata#89

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 1, 2015

@danlipsa when you added the test if these pass happily then let's merge.

doutriaux1 added a commit that referenced this pull request Dec 2, 2015

Merge pull request #1706 from UV-CDAT/continents-off
BUG #1106: wrong dataset created from nc file with decreasing lat and…

@doutriaux1 doutriaux1 merged commit 005885b into master Dec 2, 2015

7 of 9 checks passed

continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build done.
Details
cont-int/LLNL/Linux-RH6 (FULL) running 'In Queue: 1' (Tue Dec 1 14:35:02 2015)
Details
cont-int/LLNL/Darwin-Mac (NOGUI) running 'ctest -j4 -D Experimental' (Tue Dec 1 15:59:19 2015)
Details
cont-int/LLNL/Darwin-Mac LEAN running 'ctest -j4 -D Experimental' (Tue Dec 1 14:48:13 2015)
Details
cont-int/LLNL/Darwin-Mac2 10.10.5 (FULL) running 'ctest -j4 -D Experimental' (Tue Dec 1 16:25:36 2015)
Details
cont-int/LLNL/Linux-RH6 (MESA) running 'ctest -j12 -D Experimental' (Tue Dec 1 15:02:03 2015)
Details
cont-int/LLNL/Linux-Ub. 15.10 (FULL/MESA) running 'ctest -j15 -D Experimental' (Tue Dec 1 15:25:51 2015)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@doutriaux1 doutriaux1 deleted the continents-off branch Dec 2, 2015

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 2, 2015

yes look at the baseline they are "cut" at the greenwich meridian, but that's another knownish issue.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Dec 2, 2015

@doutriaux1 @aashish24 OK, I'll add a test for the sea ice.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Dec 2, 2015

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Dec 2, 2015

@doutriaux1 The aeqd projection rendering issue is #1462

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Dec 2, 2015

One thing that is strange to me is why did none of the isofill tests fail with this change? The baselines were just updated for boxfill tests.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Dec 2, 2015

@sankhesh I think isofill use pointData rather than cellData.

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Dec 2, 2015

Yes, but this change does not have anything to do with the representation. As far as I can tell, it just changes the data extents.

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