Fix "_p_void" values in TVTK with VTK 6.x #370

kitchoi · 2016-04-29T11:13:38Z

I have tested this with VTK 6.2 and 6.3 on Ubuntu 12, and VTK 6.3 on OSX 10.11

Things seem to work but since this is a fundamental change, I would prefer sitting on it for a bit longer. Review and comments are very much appreciated.

Most of these changes are tested by the existing TVTK test for initialising all classes, but I think I could also add more tests to make it more robust.

To compare the bahaviour, try the following with the master branch and with this one:

from tvtk.api import tvtk
obj = tvtk.ViewTheme()
obj.configure_traits()

…default values

codecov-io · 2016-04-29T11:13:40Z

Current coverage is 45.53%

Merging #370 into master will decrease coverage by -<.01%

@@             master       #370   diff @@
==========================================
  Files           258        258          
  Lines         22909      23011   +102   
  Methods           0          0          
  Messages          0          0          
  Branches       3030       3036     +6   
==========================================
+ Hits          10451      10476    +25   
- Misses        11729      11805    +76   
- Partials        729        730     +1

File ...ayavi/core/engine.py (not in diff) was modified. more

Powered by Codecov. Last updated by 10f5db9...c1e1f46

kitchoi · 2016-04-29T11:14:17Z

Should fix #357 and #360

prabhuramachandran · 2016-04-29T13:24:14Z

tvtk/tvtk_base.py

@@ -272,6 +274,11 @@ class TVTKBase(traits.HasStrictTraits):
    # The wrapped VTK object.
    _vtk_obj = traits.Trait(None, None, vtk.vtkObjectBase())

+    # Stores the names of the traits whose VTK Get methods may return
+    # invalid values (e.g. reference to a point) or uninitialised values
+    # We would try to update but allow it to failure


"allow it to fail" :)

…get_trait_def

stefanoborini · 2016-05-04T10:45:45Z

This is too much code to review without being told more.

kitchoi · 2016-05-04T10:50:48Z

If you try to run the new tvtk test case with the master branch + VTK 6.3, you should see a bunch of failures: https://gist.github.com/kitchoi/8e92ad08e44334133360536ce3d5bdff
These are what this PR is fixing, plus a bunch of refactoring.

stefanoborini · 2016-05-04T10:52:57Z

The problem now is that it's very hard to differentiate between what you changed for the fix and what you refactored.

stefanoborini · 2016-05-04T10:57:06Z

I think it's time we ditch codecov until they fix this unexpected changes problem.

kitchoi · 2016-05-04T10:58:00Z

If you want less of the refactoring changes, try git diff the master and this commit: 7a30bdd

stefanoborini · 2016-05-04T10:58:42Z

Try pushing a dummy change. I disabled codecov

kitchoi · 2016-05-04T16:49:14Z

tvtk/wrapper_gen.py

-                if klass.__name__ == 'vtkCubeAxesActor2D' and \
-                       name == 'inertia':
-                    # VTK bug.  Inconsistent API!
-                    rng = (float(rng[0]), float(rng[1]))


I have removed the special handling of 'vtkCubeAxesActor2D.inertia' here because from what I have researched on the VTK API, inertia is indeed an int and there is no inconsistency.

It is quite possible this was a bug with VTK 4.x or so which is not relevant anymore so this is fine.

stefanoborini · 2016-05-04T23:33:06Z

apparently the codecov yaml file did not solve for this branch

kitchoi · 2016-05-05T07:47:07Z

I think the "codecov/project" captures the increase in coverage after we added VTK 6.3 and VTK 7 in the test matrix, it raises a red flag because it did not expect those changes (who knows how they decide what they should expect).

stefanoborini · 2016-05-05T15:02:15Z

tvtk/vtk_parser.py

@@ -295,7 +294,8 @@ def get_other_methods(self):
        """
        return self.other_meths

-    def get_method_signature(self, method):
+    @staticmethod
+    def get_method_signature(method):


There are many occurrence of parser = VTKMethodParser(); parser.get_method_signature(...) in wrapper_gen and in tests (and possibly some other places I have not touched), could do with a separately cleanup PR in order to move this function out of the class?

Leave it as is. not top priority. If you want, open a issue tagged refactoring.

stefanoborini · 2016-05-05T15:22:14Z

tvtk/wrapper_gen.py

+        self._write_trait(out, name, t_def, vtk_set_meth, mapped=False)
+
+    def _write_hardware_selector_prop_color_value(self, klass, out,
+                                                  vtk_attr_name='PropColorValue'):


Weird that you have to pass a default value.

I don't have to. But I think it acts as a form of documentation.

I understand the intent, but it's deceiving. When I see that, I don't expect it to be documentation.
You already enforce the value in the content. When I see that I assume that the method can be called without any passed vtk_attr_name for some reason, and you are "fixing" that behavior with the only acceptable value, but the vtk_attr_name is always passed, only in this routine must be one and only one value, otherwise there's a programming error.

I have removed the default value now. Unfortunately I have to keep the signature consistent, so vtk_attr_name is kept there.

stefanoborini · 2016-05-05T15:23:56Z

Review ready. Merge when done (they are mostly minor).

stefanoborini · 2016-05-05T15:28:15Z

How is this related to issue #385 ? did you skip the test?

kitchoi · 2016-05-05T15:31:38Z

What is issue #907?

stefanoborini · 2016-05-05T15:38:15Z

Fixed. I pasted the wrong content.

kitchoi · 2016-05-05T16:04:41Z

tvtk/wrapper_gen.py

+                    # e.g. x = numpy.empty(9); ImageConvolve.GetKernel3x3(x)
+                    # FIXME: we could try harder in retrieving these values
+                    # in TVTKBase.update_traits
+                    del updateable_traits[name]


If I change del updateable_traits[name] to allow_update_failure.add(name), the difference in behaviour is not only that TVTKBase.update_traits will have more updating to do while those updating will almost certainly fail with TraitError, but I will also get these traits visible on the UI.
E.g. with del updateable_traits[name]
obj = tvtk.ImageConvolve(); obj.configure_traits()

E.g. replace with allow_update_failure.add(name)

Question is, which one do we want. The bahaviour on the interactive interpreter is the same.

Corran suggested using TextEditor for these arrays. Unfortunately I am preoccupied at the moment. I think I would attack this problem in a separate PR if and after this one is merged.

Fair enough. Maybe adding an issue for that would be good so we don't forget.

kitchoi · 2016-05-05T16:10:06Z

How is this related to issue #385 ? did you skip the test?

I did not do anything about #385, the error is still there (look at VTK 6.3 and VTK 7)
https://travis-ci.org/enthought/mayavi/jobs/128037832#L1208

…ove error message

prabhuramachandran · 2016-05-12T12:31:04Z

tvtk/tests/test_tvtk.py

+    def test_spider_plot_actor(self):
+        """ Test SpiderPlotActor Get/SetAxisLabel works
+        """
+        actor = tvtk.SpiderPlotActor()


It would be nice if there were a comment as to why this object is tested that would be helpful.

kitchoi · 2016-05-12T12:36:14Z

Note to self
TODO:

Test with Python 3

kitchoi · 2016-05-12T12:45:15Z

tvtk/wrapper_gen.py

+        return 'traits.Array', '', kwargs
+
+    elif type_ is bool:
+        return 'traits.Bool', '', kwargs


note to self, return 'traits.Bool', str(value), kwargs

prabhuramachandran · 2016-05-13T15:03:04Z

tvtk/wrapper_gen.py

+
+        if vtk_attr_name != 'PropColorValue':
+            raise RuntimeError("Not sure why you ask for me! "
+                               "I only deal with PropColorValue. Panicking.")


Whoa, why is this needed?

This is to prevent this method being used with vtk_attr_name other than PropColorValue. This may be unnecessary given the method is private and is used internally.

prabhuramachandran · 2016-05-13T18:14:57Z

Thanks for this, it looks nice and the code looks a lot cleaner than before! I will merge this for now.

kitchoi added 4 commits April 29, 2016 10:20

added _allow_update_failure_ to tvtk_base

32dbd6e

make VTKMethodParser.get_method_signature a staticmethod

7f5d4d4

refactored special cases for _gen_get_set_method and handled _p_void …

d41a95f

…default values

one syntax error for a special case for VTK 5.8

6db2ddc

prabhuramachandran reviewed Apr 29, 2016
View reviewed changes

kitchoi added 10 commits April 29, 2016 15:16

Merge branch 'master' into fix/357-360-tvtk-with-undefined-traits

1e4b819

absorb get_array_meta to get_trait_type and rename get_trait_type to …

7c8f967

…get_trait_def

remove debugging print

7a30bdd

wrapper_gen refactoring

f03798b

fix language in comment

a68b520

added tests for two tvtk classes

07c9f9d

this would register kernel to be a traits.Array

1ea5cce

clean up and add comments

559c567

added more tests for tvtk

117b4d4

fix error message missing formatting input

10a3838

kitchoi changed the title ~~Fix "_p_void" values in TVTK with VTK 6.x [WIP]~~ Fix "_p_void" values in TVTK with VTK 6.x May 4, 2016

kitchoi added 2 commits May 4, 2016 14:16

Merge branch 'master' into fix/357-360-tvtk-with-undefined-traits

8f07d82

cols meant to be a min between 3 and length, not max

ad538ef

kitchoi reviewed May 4, 2016
View reviewed changes

stefanoborini reviewed May 5, 2016
View reviewed changes

remove redundant test case

7088434

stefanoborini reviewed May 5, 2016
View reviewed changes

kitchoi added 3 commits May 5, 2016 16:40

add docstring

b619f45

the default value is not neccessary, nor does it matter

c95e5ab

improved docstring for get_trait_def

f88c7c8

kitchoi reviewed May 5, 2016
View reviewed changes

make docstring message on 'Raises' consistent with the code, and impr…

286e27c

…ove error message

prabhuramachandran reviewed May 12, 2016
View reviewed changes

kitchoi reviewed May 12, 2016
View reviewed changes

kitchoi added 3 commits May 12, 2016 15:38

added comments to the test for spider_actor set_axis_label

350c1f6

make kwargs a dict

491f88e

make traits.Bool have a default value too

c1e1f46

prabhuramachandran reviewed May 13, 2016
View reviewed changes

prabhuramachandran merged commit 904204d into master May 13, 2016

kitchoi deleted the fix/357-360-tvtk-with-undefined-traits branch June 29, 2016 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "_p_void" values in TVTK with VTK 6.x #370

Fix "_p_void" values in TVTK with VTK 6.x #370

kitchoi commented Apr 29, 2016

codecov-io commented Apr 29, 2016 •

edited

kitchoi commented Apr 29, 2016

prabhuramachandran Apr 29, 2016

stefanoborini commented May 4, 2016

kitchoi commented May 4, 2016

stefanoborini commented May 4, 2016

stefanoborini commented May 4, 2016

kitchoi commented May 4, 2016

stefanoborini commented May 4, 2016

kitchoi May 4, 2016

prabhuramachandran May 13, 2016

stefanoborini commented May 4, 2016

kitchoi commented May 5, 2016

stefanoborini May 5, 2016

kitchoi May 5, 2016

stefanoborini May 5, 2016

stefanoborini May 5, 2016

kitchoi May 5, 2016

stefanoborini May 5, 2016

kitchoi May 5, 2016

stefanoborini commented May 5, 2016

stefanoborini commented May 5, 2016 •

edited

kitchoi commented May 5, 2016

stefanoborini commented May 5, 2016

kitchoi May 5, 2016 •

edited

kitchoi May 6, 2016

prabhuramachandran May 13, 2016 •

edited

kitchoi commented May 5, 2016

prabhuramachandran May 12, 2016

kitchoi commented May 12, 2016

kitchoi May 12, 2016

prabhuramachandran May 13, 2016

kitchoi May 13, 2016

prabhuramachandran commented May 13, 2016

Fix "_p_void" values in TVTK with VTK 6.x #370

Fix "_p_void" values in TVTK with VTK 6.x #370

Conversation

kitchoi commented Apr 29, 2016

codecov-io commented Apr 29, 2016 • edited

Current coverage is 45.53%

kitchoi commented Apr 29, 2016

Choose a reason for hiding this comment

stefanoborini commented May 4, 2016

kitchoi commented May 4, 2016

stefanoborini commented May 4, 2016

stefanoborini commented May 4, 2016

kitchoi commented May 4, 2016

stefanoborini commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanoborini commented May 4, 2016

kitchoi commented May 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanoborini commented May 5, 2016

stefanoborini commented May 5, 2016 • edited

kitchoi commented May 5, 2016

stefanoborini commented May 5, 2016

kitchoi May 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prabhuramachandran May 13, 2016 • edited

Choose a reason for hiding this comment

kitchoi commented May 5, 2016

Choose a reason for hiding this comment

kitchoi commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prabhuramachandran commented May 13, 2016

codecov-io commented Apr 29, 2016 •

edited

stefanoborini commented May 5, 2016 •

edited

kitchoi May 5, 2016 •

edited

prabhuramachandran May 13, 2016 •

edited