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

VCS Addons: histogram & polar #1999

Closed

Conversation

chaosphere2112
Copy link
Contributor

Fixes up the existing implementation of the vcsaddons histogram and adds a new plot type (Polar) to vcsaddons. The histogram came out of needs of the GUI, and the polar plot came from some visualization tasks I had been assigned. New baselines are at CDAT/uvcdat-testdata#138, here's the coverage summary for these two modules:

screen shot 2016-05-31 at 12 00 26 pm
screen shot 2016-05-31 at 12 00 30 pm

@chaosphere2112
Copy link
Contributor Author

@aashish24 @doutriaux1 Please review, I want this in 2.6.

@doutriaux1
Copy link
Contributor

@chaosphere2112 is merging @danlipsa revert commit branch going to affect this?

@doutriaux1
Copy link
Contributor

@chaosphere2112 please merge master in

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 Merged, running tests now.

@chaosphere2112
Copy link
Contributor Author

Looks good. Also, the commit from @sampsonbryce only made it so that calling x.plot(s, vcsaddon) worked, where in these tests I do vcsaddon.plot(s, x=x)

import vcs
import vcsaddons, numpy

x=regression.init()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick: space before and after =

@aashish24
Copy link
Contributor

y0 = center[1] + (ymul * radius * numpy.sin(angle))
y1 = center[1]
if t_labels is not None:
label = self.create_text(template.xlabel1.texttable, self.text_orientation_for_angle(angle, source=template.xlabel1.textorientation))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we wrap these long lines please (that are > 120 column)

@aashish24
Copy link
Contributor

@chaosphere2112 I did some basic check, couldn't verify all the math (specially the edge cases) but this seems like a nice addition. I just some very minor suggestions, please have look.

@chaosphere2112
Copy link
Contributor Author

@aashish24 Updated style issues. The baseline you linked looks fine; if you're referring to the line stippling, the issue is just that the line stipples are overlapping (each bar has its own outline), so they look solid.

@aashish24
Copy link
Contributor

Okay, thanks, LGTM now 👍 Do I need to build and test?

@chaosphere2112
Copy link
Contributor Author

@aashish24 I ran the tests and they passed for me, but, probably 😉

@aashish24
Copy link
Contributor

Okay running it now.

@aashish24
Copy link
Contributor

@chaosphere2112 my build is done but couldn't finish the testing, will do it tonight.

@aashish24
Copy link
Contributor

@chaosphere2112 for some reason I got more than expected test failures 😢. Let me look into it and I will ping you asap.

@aashish24
Copy link
Contributor

@chaosphere2112 the baselines were not uptodate in the branch and thats why tests were failing. I am running the suite again.

@aashish24
Copy link
Contributor

Also, I have merged the master baselines to this branch and running the tests again.

@aashish24
Copy link
Contributor

these tests are failing but I am going to ignore dv3d and diag tests as they have been failing on master of this branch too.

40 - flake8_vcs (Failed)
60 - test_vcs_read_old_scr (Timeout)
62 - test_vcs_read_old_scr_2 (Timeout)
65 - test_vcs_no_extra_elements (Failed)
542 - dv3d_vector_test (Failed)
543 - dv3d_slider_test (Failed)
544 - Hovmoller_volume_test (Failed)
545 - dv3d_volume_test (Failed)
546 - dv3d_constituents_test (Failed)
547 - dv3d_surface_test (Failed)
548 - dv3d_hovmoller_test (Failed)
594 - CDMS_Test_multiple_formats (Failed)
606 - diags_test_02 (Failed)
607 - diags_test_03 (Failed)
608 - diags_test_04 (Failed)
609 - diags_test_41 (Failed)
610 - diags_test_05 (Failed)
611 - diags_test_06 (Failed)
612 - diags_test_07 (Failed)
613 - diags_test_08 (Failed)
614 - diags_test_09 (Failed)
615 - diags_test_10 (Failed)
616 - diags_test_11 (Failed)
617 - diags_test_12 (Failed)
618 - diags_test_13 (Failed)
619 - diags_test_15 (Failed)

Looking into other tests failure making sure that they are not related.

@aashish24
Copy link
Contributor

@chaosphere2112 looks like polared_one broke one test:

57: Test command: /home/chaudhary/tools/uvcdat/build_debug/install/bin/runtest "/home/chaudhary/tools/uvcdat/build_debug/install/bin/python" "/home/chaudhary/tools/uvcdat/src.git/testing/vcs/test_vcs_no_extra_elements.py"
57: Environment variables:
57: UVCDAT_ANONYMOUS_LOG=no
57: Test timeout computed to be: 1500
57: PATH env variable reset
57: LD_LIBRARY_PATH env variable reset
57: DYLD_LIBRARY_PATH env variable reset
57: PYTHONPATH env variable reset
57: OPAL_PREFIX env variable reset
57: LIBOVERLAY_SCROLLBAR env variable reset
57: Successfully updated your environment to use UVCDAT
57: (changes are valid for this session/terminal only)
57: Version: v2.5.0-407-ga90268e
57: Location: /home/chaudhary/tools/uvcdat/build_debug/install
57: Reset these changes by running: source /home/chaudhary/tools/uvcdat/build_debug/install/bin/reset_runtime.sh
57: template ['polar_oned']
57: textorientation ['__textorientation_21042658900488']
57: line ['__line_320635056441535', '__line_957320724349839']
57: Traceback (most recent call last):
57: File "/home/chaudhary/tools/uvcdat/src.git/testing/vcs/test_vcs_no_extra_elements.py", line 35, in
57: raise Exception("New elements added when it shouldn't")
57: Exception: New elements added when it shouldn't
1/1 Test #57: test_vcs_no_extra_elements .......***Failed 1.64 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) = 1.68 sec

@chaosphere2112
Copy link
Contributor Author

@aashish24 Ah, I could see that. I fixed the flake8 issue, I'll address the polar_oned thing.

@chaosphere2112
Copy link
Contributor Author

Take another shot at it, @aashish24

@aashish24
Copy link
Contributor

thanks @chaosphere2112 running the suite from scratch again with your latest changes.

@aashish24
Copy link
Contributor

@chaosphere2112, apologies, I am pushing a fix since lot many tests were failing on my system. Also, I will open a PR against uvcdat branch.

@aashish24 aashish24 closed this Jun 11, 2016
@aashish24 aashish24 mentioned this pull request Jun 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants