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

Fixes failing DV3D tests and VCS behavior #2003

Merged
merged 5 commits into from
Jun 8, 2016
Merged

Fixes failing DV3D tests and VCS behavior #2003

merged 5 commits into from
Jun 8, 2016

Conversation

chaosphere2112
Copy link
Contributor

One small change in the DV3D package (changes how inherited parameters are initialized), some re-jiggering of logic in vcs/dv3d.py, and a small tweak in the testing framework.

  1. I updated the VCS integration to properly obey the way we handle parent/child relationships (where the child inherits all attributes from the parent and then is a separate entity). This appears to work correctly.
  2. Fixed the 3d_scalar/3d_vector/3d_dual_scalar methods to pass the argument they received instead of just "default" as source
  3. I made axes be inherited from the parent as well (for children of Hovmoller3D).

The commit (9404c34) that broke the integration did so because we can now instantiate a new DV3D graphics method using the vcs.creategraphicsmethod function (which seems reasonable to me).

The way that this broke everything was that previously, that function returned None, which we later compared against in the canvas.__plot function and decided to just use the original graphics method (presumably a fallback of some sort). This made the behavior of the 3D plots emulate the normal DV3D behavior (where multiple cells are linked together), but violate the standard VCS behavior (where everything is separate).

We can argue about that behavior separately; this pull request does not affect it at all, and I think that the offending commit is also fine (because the functionality it adds is good). If we want the old behavior, we should rewrite the logic in __plot so the change is in the relevant place, rather than crippling other functionality because it happens to tweak the behavior.

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 @aashish24 @danlipsa Please review.

@danlipsa
Copy link
Contributor

danlipsa commented Jun 2, 2016

@chaosphere2112 Looks reasonable. I am OK with merging this if all but the diags tests pass.

@danlipsa
Copy link
Contributor

danlipsa commented Jun 3, 2016

@doutriaux1 @aashish24 Ping. Can we merge this? I don't like to have failing tests on the master branch.

@doutriaux1
Copy link
Contributor

@chaosphere2112 it fails for me after I merged master in. @danlipsa please try to merge master in and run from scratch. If it works for you then go ahead and merge. Thanks.

@danlipsa
Copy link
Contributor

danlipsa commented Jun 3, 2016

@chaosphere2112 @doutriaux1 Indeed dv3d_ still fail for me as well. @chaosphere2112 any suggestions?

@chaosphere2112
Copy link
Contributor Author

@danlipsa @doutriaux1 What are the test failures you're getting? Sorry, been out.

@danlipsa
Copy link
Contributor

danlipsa commented Jun 7, 2016

@chaosphere2112 This is the file generated on my machine for dv3d_vector_test. Don't you get the same results?
dv3d_vector_test test

@chaosphere2112
Copy link
Contributor Author

@danlipsa Nope, it passes for me. Are you building from scratch, or just updating the vcs module? If the latter, you also need to update the DV3D module.

@doutriaux1
Copy link
Contributor

@chaosphere2112 remember it didn't work for me either running from scratch. I'll merge master in from here, maybe that will help.

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 I'll try and build from scratch to see if it stops working for me.

@danlipsa
Copy link
Contributor

danlipsa commented Jun 8, 2016

@chaosphere2112 Thanks, updating DV3D made it pass for me. So, I am fine with merging this 👍

@doutriaux1
Copy link
Contributor

@danlipsa @chaosphere2112 if it works for both of you go ahead and merge.

@danlipsa danlipsa merged commit 6e74cf5 into master Jun 8, 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