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

fix #397 needed to store continent value #398

Merged
merged 4 commits into from Apr 1, 2019

Conversation

doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Mar 28, 2019

No description provided.

@doutriaux1 doutriaux1 requested a review from scottwittenburg Mar 28, 2019
@doutriaux1 doutriaux1 added the bug label Mar 28, 2019
@doutriaux1 doutriaux1 added this to the 8.2 milestone Mar 28, 2019
class TestVCSTextsExtents(basevcstest.VCSBaseTest):
def testUpdateDoesNotTriggerContinents(self):
s = self.clt["clt"][0]
self.x.plot(s, continents=0)
Copy link
Collaborator

@scottwittenburg scottwittenburg Mar 28, 2019

Choose a reason for hiding this comment

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

Interesting, I guess while I knew you could disable the continents somehow, I never knew this is how you did it.

@@ -4054,6 +4054,8 @@ def __plot(self, arglist, keyargs):
dn.g_name = arglist[4]
dn.array = arglist[:2]
dn.backend = returned_kargs
if "continents" in keyargs:
dn._continents = keyargs["continents"]
Copy link
Collaborator

@scottwittenburg scottwittenburg Mar 28, 2019

Choose a reason for hiding this comment

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

After browsing the source code, I'm not seeing how this value stored on the display is ever queried to avoid plotting the continents. To me it looks like the decision is made inside our vcs2vtk.genGrid(...) function, based solely on the type of data we get. But I guess it's working, so it's fine with me.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Mar 29, 2019

Choose a reason for hiding this comment

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

you're right, it's sort of obscure but i think it's there: https://github.com/CDAT/vcs/blob/master/vcs/VTKPlots.py#L491

Copy link
Collaborator

@scottwittenburg scottwittenburg Mar 29, 2019

Choose a reason for hiding this comment

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

Ah, you referred to it with an underscore in your fix, so I was searching for it that way, forgetting about how you typically make "properties" out of those attributes. Thanks.

@doutriaux1
Copy link
Contributor Author

doutriaux1 commented Mar 29, 2019

@scottwittenburg failures are in doc build/sphinx I think i'ts safe to merge from that point of view, doesn't seem related to this PR

@scottwittenburg
Copy link
Collaborator

scottwittenburg commented Mar 29, 2019

I was just searching through the CI output for "error", there were 86 occurrences, making it hard to tell what's going on. Some were image comparison size mismatches:

Executing nosetests -A not vtk_ui -s /Users/distiller/project/tests/test_vcs_dejavu_fonts.py in /Users/distiller/project
b'ERROR: In ../Imaging/Core/vtkImageDifference.cxx, line 558'
b'vtkImageDifference (0x7fd2aa9c7ad0): ExecuteInformation: Input are not the same size.'
b' Input1 is: 0,813,0,302,0,0'
b' Input2 is: 0,813,0,303,0,0'

But there are a bunch of those in the passing py2 check, but what the heck? Why don't those cause test failures? Whatever. Then there are a bunch similar to this:

CDMS error: Error on relative units conversion, string =

And again, many of the same in the py2 case. I have no idea what that's about. In both cases (check passed vs check failed), there are also some errors I've seen before coming from VTK when we do projection and an input point is outside the range it can handle:

Generic Warning: In ../Common/Core/vtkMath.cxx, line 743
vtkMath::Jacobi: Error extracting eigenfunctions

From what I can tell, though, it seems you're right that the only kind of error we see in the failing case which we don't also see in the passing case, is this one:

Extension error:
Could not import extension sphinx.builders.epub3 (exception: No module named 'sphinxcontrib')
make: *** [doctest] Error 2
Exited with code 2

Has that been happening with other PRs recently?

@coveralls
Copy link

coveralls commented Mar 29, 2019

Coverage Status

Coverage remained the same at ?% when pulling d2f70be on issue_397_update_brings_back_continents into e607721 on master.

@doutriaux1 doutriaux1 merged commit d86a55d into master Apr 1, 2019
@doutriaux1 doutriaux1 deleted the issue_397_update_brings_back_continents branch Apr 1, 2019
@downiec downiec modified the milestones: 8.2, 8.2.1 Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants