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

BUG #1944: Rename line to linetype for isoline, unified1d and vector. #2046

Merged
merged 3 commits into from Aug 10, 2016

Conversation

danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jul 6, 2016

Add separate function to set type, color and width from a line.

@danlipsa
Copy link
Contributor Author

danlipsa commented Jul 6, 2016

@doutriaux1 @aashish24 Please review. This required more changes than I thought or wanted. Overall I think it is a much clearer API.

@sankhesh
Copy link
Contributor

Do: check

@kwrobot
Copy link
Member

kwrobot commented Jul 18, 2016

Basic content checks failed!

commit 7442915e adds bad whitespace:
testing/vcs/test_vcs_dump_json.json:13: trailing whitespace.
+   "linetype": "solid", 
testing/vcs/test_vcs_dump_json.json:192: trailing whitespace.
+   ], 
testing/vcs/test_vcs_dump_json.json:228: trailing whitespace.
+   "linetype": null, 

Branch-at: 7442915
Rejected-by: @kwrobot

@danlipsa
Copy link
Contributor Author

Do: check

@kwrobot
Copy link
Member

kwrobot commented Jul 26, 2016

Basic content checks passed!

Changes since last check: compare

Branch-at: 94c840d
Acked-by: @kwrobot

@danlipsa
Copy link
Contributor Author

@doutriaux1 @aashish24 ping! This also has been setting here for too long.

@danlipsa
Copy link
Contributor Author

Do: test

@danlipsa
Copy link
Contributor Author

@sankhesh @doutriaux1 This test
https://open.cdash.org/testDetails.php?test=462274077&build=4475133
uses
ACTIVATING ENV:2.6
but should be something like:
ACTIVATING ENV:2.6-11-g94c840d
Any idea why?

@doutriaux1
Copy link
Contributor

That is odd indeed did you start with the 2.6 env ?

@danlipsa
Copy link
Contributor Author

danlipsa commented Jul 26, 2016

@doutriaux1 This is done by buildbot. @sankhesh says that he deletes both the source and the build directory so I am not sure where 2.6 is picked up from.

@danlipsa
Copy link
Contributor Author

Do: test

@kwrobot
Copy link
Member

kwrobot commented Jul 26, 2016

Basic content checks failed!

commit 1c404ce3 has committer name "GitHub" with no space.  Run
  git config --global user.name 'Your Name'
  git config --global user.email 'you@yourdomain.com'
before creating commits.

Changes since last check: compare

Branch-at: 1c404ce
Rejected-by: @kwrobot

@danlipsa
Copy link
Contributor Author

Do: test

@kwrobot
Copy link
Member

kwrobot commented Jul 26, 2016

Basic content checks passed!

Changes since last check: compare

Branch-at: 7939632
Acked-by: @kwrobot

@@ -6,16 +6,22 @@
l.type = "dash"

v=vcs.createvector()
v.line = "vcs_test_set_line"
v.setLineAttributes("vcs_test_set_line")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danlipsa what does this function do at this point in the code?

Copy link
Contributor Author

@danlipsa danlipsa Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aashish24 Sets linetype, linecolor and linewidth from the line with the name 'vcs_test_set_line'.

@danlipsa
Copy link
Contributor Author

@aashish24 Done.

except:
pass

line = property(_getline, _setline)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should break backward compatibility, we should probably keep it and raise a deprecated warning.

Copy link
Contributor Author

@danlipsa danlipsa Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doutriaux1 @aashish24 Makes sense. The line interface will still have the bug @chaosphere2112 reported. We won't do any fixes there. Maybe remove it in the next version.

@danlipsa
Copy link
Contributor Author

danlipsa commented Aug 4, 2016

These are the test failures on Linux:
96% tests passed, 27 tests failed out of 611

Total Test time (real) = 407.44 sec

The following tests FAILED:
13 - testWrite (Failed)
73 - test_vcs_verify_boxfill_basics (Failed)
121 - test_vcs_boxfill_decreasing_latitude (Failed)
122 - test_vcs_meshfill_no_wrapping (Failed)
413 - test_vcs_isofill_level0 (Failed)
414 - test_vcs_isofill_level1 (Failed)
415 - test_vcs_isofill_level2 (Failed)
469 - test_vcs_matplotlib_colormap (Failed)
549 - dv3d_vector_test (Failed)
550 - dv3d_slider_test (Failed)
551 - Hovmoller_volume_test (Failed)
552 - dv3d_volume_test (Failed)
553 - dv3d_constituents_test (Failed)
554 - dv3d_surface_test (Failed)
555 - dv3d_hovmoller_test (Failed)
580 - cdms2_test_createcopy_lose_dtype (Failed)
581 - CDMS_Test_01 (Failed)
582 - CDMS_Test_02 (Failed)
583 - CDMS_Test_03 (Failed)
584 - CDMS_Test_04 (Failed)
590 - CDMS_Test_10 (Failed)
598 - CDMS_Test_18 (Failed)
599 - CDMS_Test_20 (Failed)
602 - CDMS_Test_write_compressed (Failed)
606 - CDMS_Test_pack (Failed)
609 - CDMS_Test_write_extended_dimension (Failed)
610 - CDMS_Test_del_attributes (Failed)

@doutriaux1
Copy link
Contributor

@danlipsa no worries about the failed test, it's due to hdf5 not matching anymore by default under conda. But once I merge the legend PR you will likely need to re-update your baselines.

@doutriaux1
Copy link
Contributor

actually I take it back, this PR does NOT touch the baselines.

@danlipsa
Copy link
Contributor Author

danlipsa commented Aug 4, 2016

@doutriaux1 Indeed the failed vcs tests are because of hdf5 problem.

@danlipsa
Copy link
Contributor Author

danlipsa commented Aug 4, 2016

@doutriaux1 @aashish24 Lets get this merged as well, unless you have additional comments/concerns.

@doutriaux1
Copy link
Contributor

@danlipsa I would like to finish/merge fix_travis first if it doesn't take too long.

@cdatrobot
Copy link

Basic content checks failed!

commit b24b2c9e has committer name "GitHub" with no space.  Run
  git config --global user.name 'Your Name'
  git config --global user.email 'you@yourdomain.com'
before creating commits.
commit e20fded7 adds bad whitespace:
testing/vcs/test_vcs_dump_json.json:13: trailing whitespace.
+   "linetype": "solid", 
testing/vcs/test_vcs_dump_json.json:192: trailing whitespace.
+   ], 
testing/vcs/test_vcs_dump_json.json:228: trailing whitespace.
+   "linetype": null, 

Branch-at: b24b2c9
Rejected-by: @llnlbot

@danlipsa
Copy link
Contributor Author

danlipsa commented Aug 4, 2016

@doutriaux1 Do you worry about conflicts/broken tests?

@doutriaux1
Copy link
Contributor

kind of the opposite, since it shouldn't break anything I would like to see a PR pass on travis ;)

@danlipsa
Copy link
Contributor Author

danlipsa commented Aug 4, 2016

@doutriaux1 I guess my point was that even if you merge this your PR on travis can go on as before. You should not have any conflicts or any new failing tests. But I can wait if you want to take a closer look at this and you want to finish the travis work first without interruption.

@cdatrobot
Copy link

Basic content checks failed!

commit e20fded7 adds bad whitespace:
testing/vcs/test_vcs_dump_json.json:13: trailing whitespace.
+   "linetype": "solid", 
testing/vcs/test_vcs_dump_json.json:192: trailing whitespace.
+   ], 
testing/vcs/test_vcs_dump_json.json:228: trailing whitespace.
+   "linetype": null, 

Branch-at: b24b2c9
Rejected-by: @llnlbot

@cdatrobot
Copy link

Basic content checks passed!

Changes since last check: compare

Branch-at: b635154
Acked-by: @llnlbot

@aashish24
Copy link
Contributor

@sankhesh I see 4 contents check but no build / test check after the last test command from @danlipsa. What's wrong?

@sankhesh
Copy link
Contributor

sankhesh commented Aug 7, 2016

@aashish24 I was puzzled by it yesterday too. I looked into it. This pull request is older than the changes we made to the bot and I am assuming buildbot is getting confused with the different commands on it. I'll talk to Ben on Monday to see what's wrong exactly.

@danlipsa
Copy link
Contributor Author

danlipsa commented Aug 9, 2016

@sankhesh It seems that some of the tests still do not activate the right environment:
https://open.cdash.org/testDetails.php?test=462274077&build=4495107

@sankhesh
Copy link
Contributor

sankhesh commented Aug 9, 2016

@danlipsa All the tests on default build are activating the right environment. The tests on the lean build just activate 2.6. I think that is a UV-CDAT issue that needs to be fixed in the code.

@danlipsa
Copy link
Contributor Author

danlipsa commented Aug 9, 2016

@sankhesh How do you see the default build? I only see the lean build - I click on cdash

@sankhesh
Copy link
Contributor

Here is the default build: https://open.cdash.org/buildSummary.php?buildid=4495026

@danlipsa
Copy link
Contributor Author

@sankhesh The test failures look like @doutriaux1 colorbar baselines. I will run the test on my machine to see if they pass.

@danlipsa
Copy link
Contributor Author

danlipsa commented Aug 10, 2016

@sankhesh On my machine most tests pass - I have 26 failures - most vcs once are from hdf5.
Here are @doutriaux1 baselines.
https://github.com/UV-CDAT/uvcdat-testdata/pull/149/files

In the link you provided, it looks like @doutriaux1 changes are not in, possible because the code uses the wrong conda env? The SHA is correct. The baseline is right but the generated image is wrong. Can you look in the LastTest.log to see the conda env for the tests?

@doutriaux1
Copy link
Contributor

Do: test

@cdatrobot
Copy link

Testing commands handed to buildbot.

Branch-at: b635154

@cdatrobot
Copy link

Your merge request has been queued for testing. You may view the test results on CDash or Buildbot.

Branch-at: b635154

@doutriaux1 doutriaux1 merged commit 0f3f507 into master Aug 10, 2016
@doutriaux1 doutriaux1 deleted the linestyle branch August 10, 2016 21:21
chaosphere2112 pushed a commit that referenced this pull request Sep 1, 2016
…#2046)

* BUG #1944: Rename line to linetype for isoline, unified1d and vector.

Add separate function to set type, color and width from a line.

* Fix test_vcs_read_old_scr_2 - we have one less template
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants