Operator results #567

cquammen · 2016-08-22T20:46:13Z

This branch introduces operator results, data sets that are "byproducts" of an operator. Examples include spreadsheets containing information about segmented objects and spreadsheets with algorithm execution information, but any vtkDataObject can be added as an artifact.

Additionally, this branch introduces a first cut at operator descriptions in JSON. The Connected Components operator serves as an example.

Addresses #497, #498, and partially #525.

cquammen · 2016-08-22T20:52:44Z

To test:

Load SMALL image data set
Run Connected Components, accept default values. Notice the item named "Component Statistics" in the pipeline browser

Click the
button and select SpreadSheet View.
From the "Showing" combo box, choose "TrivialProducer2". You will see SurfaceArea, SurfaceAreaToVolumeRatio, and Volume columns in a spreadsheet view.

Hovden · 2016-08-22T20:56:12Z

Try not let this side track the discussion, this is just a comment on nomenclature. "Artifacts" has a really bad meaning and we very often discuss them in tomography / visualization. Can we give it a new name? I'm open to anything, some suggestions: "Child", "Output", "ProcessedData", "Offspring", "Creation".

cquammen · 2016-08-22T21:06:55Z

I'm not attached to "artifact", and am open to suggestions. Ideally, the name would differentiate these kinds of secondary outputs from the main result of the operator. Do you have a word that you use for a secondary result or secondary analysis?

cquammen · 2016-08-22T21:12:24Z

How about "Product" or "Result"?

Hovden · 2016-08-22T22:24:30Z

"Result" sounds good.

This class is a wrapper object for new data outputs produced by an Operator.

This commit adds member functions to specify the number of artifacts an operator produces and to set those artifacts from VTK data objects.

cquammen · 2016-08-23T03:03:22Z

Name changed from "Artifact" to "Result"

cquammen · 2016-08-23T18:34:33Z

@cryos ready for review when you get a chance

cquammen · 2016-08-23T19:56:02Z

@cjh1 Mind taking a look?

cjh1 · 2016-08-23T19:59:47Z

Sure, I will take a look now.

cjh1 · 2016-08-23T20:08:02Z

@cquammen Do I need a particular version of ITK, I am getting the following error:

Exception encountered while running ConnectedComponents
'itkTemplate' object has no attribute 'US3'

cjh1 · 2016-08-23T20:14:26Z

I am also seeing the following compiler warnings ( not sure if this PR introduced all of them ):

/home/cjh/work/source/tomviz/tomviz/OperatorPython.cxx: In member function ‘virtual bool tomviz::OperatorPython::applyTransform(vtkDataObject*)’:
/home/cjh/work/source/tomviz/tomviz/OperatorPython.cxx:248:23: warning: declaration of ‘result’ shadows a previous local [-Wshadow]
       OperatorResult *result = resultAt(i);
                       ^~~~~~
/home/cjh/work/source/tomviz/tomviz/OperatorPython.cxx:228:20: note: shadowed declaration is here
   vtkSmartPyObject result;
                    ^~~~~~
/home/cjh/work/source/tomviz/tomviz/OperatorPython.cxx:244:15: warning: unused variable ‘key’ [-Wunused-variable]
     PyObject *key, *value;
               ^~~
/home/cjh/work/source/tomviz/tomviz/OperatorPython.cxx:244:21: warning: unused variable ‘value’ [-Wunused-variable]
     PyObject *key, *value;
                     ^~~~~
/home/cjh/work/source/tomviz/tomviz/OperatorPython.cxx:245:16: warning: unused variable ‘pos’ [-Wunused-variable]
     Py_ssize_t pos = 0;

cquammen · 2016-08-23T20:17:36Z

You will need to have ITK built with the following types wrapped:

ITK_WRAP_VECTOR_COMPONENTS:STRING=3
ITK_WRAP_complex_double:BOOL=OFF
ITK_WRAP_complex_float:BOOL=ON
ITK_WRAP_covariant_vector_double:BOOL=OFF
ITK_WRAP_covariant_vector_float:BOOL=OFF
ITK_WRAP_double:BOOL=OFF
ITK_WRAP_float:BOOL=ON
ITK_WRAP_rgb_unsigned_char:BOOL=ON
ITK_WRAP_rgb_unsigned_short:BOOL=OFF
ITK_WRAP_rgba_unsigned_char:BOOL=ON
ITK_WRAP_rgba_unsigned_short:BOOL=OFF
ITK_WRAP_signed_char:BOOL=ON
ITK_WRAP_signed_long:BOOL=OFF
ITK_WRAP_signed_short:BOOL=ON
ITK_WRAP_unsigned_char:BOOL=ON
ITK_WRAP_unsigned_long:BOOL=ON
ITK_WRAP_unsigned_short:BOOL=ON
ITK_WRAP_vector_double:BOOL=OFF
ITK_WRAP_vector_float:BOOL=ON

cquammen · 2016-08-23T20:23:58Z

Compiler warnings are new. Fixing...

cjh1 · 2016-08-23T20:28:21Z

Should we update the ITK build instructions to include those options?

mathturtle · 2016-08-23T20:32:53Z

Yes, and I will probably need to update the prebuilt ITK that the tomviz superbuild uses.

cjh1 · 2016-08-23T20:38:33Z

After changing those options my ITK configure gives me:

 Warning: No template declared for itk::EdgePotentialImageFilter. Perhaps you
 should turn on more WRAP_* options?

 Warning: No template declared for itk::VectorMagnitudeImageFilter. Perhaps
 you should turn on more WRAP_* options?

 Warning: No template declared for
 itk::DifferenceOfGaussiansGradientImageFilter. Perhaps you should turn on
 more WRAP_* options?

 Warning: No template declared for itk::GradientImageFilter. Perhaps you
 should turn on more WRAP_* options?

The build now fails.

cquammen · 2016-08-23T20:42:27Z

Hrm... I will send you my ITK CMakeCache.txt file. I am building ITK 4.9rc01 btw. 4.9 should be fine, though.

cjh1 · 2016-08-23T20:44:25Z

Ok, thanks, I'm using 4.9

VTK objects can be created in Python and returned to the C++ side by returning a dictionary of values from the 'transform_scalars' function. It is assumed that each key in the dictionary has a vtkObject value that will be wrapped by an OperatorResult.

Added function to read in a text file with a given file root and an extension. Made existing readInPythonScript function use this function and added a readInJSONDescription function to load JSON text files.

Use that support for loading the JSON file in the Connected Components filter.

This is a handy function to create vtkTables from 2D numpy tables. Modified ConnectedComponents.py to use the function.

thewtex · 2016-08-24T14:17:10Z

@cjh1 @cquammen I will try to reproduce locally...

thewtex · 2016-08-24T20:10:08Z

WIP for issue is here: http://review.source.kitware.com/#/c/21484/

cquammen · 2016-08-24T23:00:15Z

@cjh1 For now, you should also be able to leave the option -DITK_WRAP_VECTOR_COMPONENTS to the default, or set it back to -DITK_WRAP_VECTOR_COMPONENTS:STRING=2;3;4.

cjh1 · 2016-08-25T00:48:03Z

@cquammen Thanks, I was able to compile ITK using that default.

cjh1 · 2016-08-25T01:14:51Z

@thewtex Thanks for taking a look at this.

cjh1 · 2016-08-25T12:43:08Z

tomviz/Operator.cxx

+{
+  int previousSize = m_results.size();
+  if (previousSize < n)
+  {


Opening brace on line above ( there are a couple of places )

cjh1 · 2016-08-25T12:51:57Z

@cquammen Code looks good, just a few minor formatting issue ( being on clang-format :-) ). Tested it out and works as expected. I have a few question:

Currently you can right click on the operator result and it has the option to "Delete", should we disable the right click?
What exactly are the operator descriptions being used for?
In the future are we going to allow operators to be applied to operator results?
Who own's the vtkDataObject's associated with the result object? How do they get cleaned up?

cquammen · 2016-08-25T13:56:53Z

Thanks for taking a look, @cjh1.

Currently you can right click on the operator result and it has the option to "Delete", should we disable the right click?

Yes. It would also be good to add some kind of table/spreadsheet icon for vtkTable results, and bring up a SpreadSheet View or update an open SpreadSheet view when it is left-clicked on. I was wondering if you might want to tackle that (we should discuss the behavior with the larger group first).

What exactly are the operator descriptions being used for?

@cryos and I have talked about describing operators in JSON for automatic UI generation, enabling/disabling menu items and toolbar items, etc. Think ParaView filter descriptions in XML, just in JSON and tomviz specific. The examples I have provided are not set in stone - they're just a proposal for what the JSON files might look like. Marcus has some sketched out, too, I believe. I wanted to get a couple in here mainly to describe the operator results.

In the future are we going to allow operators to be applied to operator results?

Excellent question. So far, I believe we have decided further operations will not be allowed on operator results. However, I will next work on supporting "child" data sets, such as binary label maps from image segmentation algorithms. Operators will be allowed on these child data sets.

Who own's the vtkDataObject's associated with the result object? How do they get cleaned up?

The OperatorResult class owns it. It unregisters the proxy associated with the vtkDataObject from the proxy manager in its destructor.

cryos · 2016-08-25T13:59:32Z

It would be nice to consider what new build options we need in ITK, I can test these locally too, document them, and then get the ITK in the superbuild updated assuming we need them all. Ideally it would be good to stick to the things we really need in ITK, if that is all the new options you added then that is fine, but if some are there and unlikely to be required I would prefer to cut the list down/have a simpler ITK.

This is looking great, will try and get it compiled later. Looking forward to getting some chart views of this too. What operators do you want on child data sets? I thought just visualization modules were planned at this stage.

cquammen · 2016-08-25T14:00:29Z

Oh, I thought @Hovden wanted to be able to apply operators on child data sets. I might have misunderstood.

cjh1 · 2016-08-25T14:00:50Z

Yes. It would also be good to add some kind of table/spreadsheet icon for vtkTable results, and bring up a SpreadSheet View or update an open SpreadSheet view when it is left-clicked on. I was wondering if you might want to tackle that (we should discuss the behavior with the larger group first).

Would be happy to.

cjh1 · 2016-08-25T14:02:07Z

The reason I asked about operator on operator result as it may have an impact on the threading work I am doing ...

cquammen · 2016-08-26T18:52:50Z

@cjh1 Did you have any luck getting this to run?

cryos · 2016-08-26T20:00:33Z

What ITK is this branch proposing to move Tomviz to, and what are the build options it is adding above and beyond what https://github.com/OpenChemistry/tomviz/blob/master/BUILDING.md asks for?

cquammen · 2016-08-26T20:33:25Z

Let's stick with v4.9.0 for now. I ran into build errors with v4.9.1, and am building 4.10.0 right now, which seems to have resolved them.

You should use the ITK you may already have built for now. If there is an error message when you run the Connected Components operator, set the type wrapping options in ITK to:

-DITK_WRAP_complex_double:BOOL=OFF
-DITK_WRAP_complex_float:BOOL=ON
-DITK_WRAP_covariant_vector_double:BOOL=OFF
-DITK_WRAP_covariant_vector_float:BOOL=OFF
-DITK_WRAP_double:BOOL=OFF
-DITK_WRAP_float:BOOL=ON
-DITK_WRAP_rgb_unsigned_char:BOOL=ON
-DITK_WRAP_rgb_unsigned_short:BOOL=OFF
-DITK_WRAP_rgba_unsigned_char:BOOL=ON
-DITK_WRAP_rgba_unsigned_short:BOOL=OFF
-DITK_WRAP_signed_char:BOOL=ON
-DITK_WRAP_signed_long:BOOL=OFF
-DITK_WRAP_signed_short:BOOL=ON
-DITK_WRAP_unsigned_char:BOOL=ON
-DITK_WRAP_unsigned_long:BOOL=ON
-DITK_WRAP_unsigned_short:BOOL=ON
-DITK_WRAP_vector_double:BOOL=OFF
-DITK_WRAP_vector_float:BOOL=ON

I intend to further investigate building the subset of ITK that we need after the segmentation work is in place. Initial attempts to turn on only some of the modules exposed some problems in the form of additional and unsatisfied module dependencies that weren't really needed. This problem should be fixed in master, apparently. Each new ITK build takes over an hour, so the configuration for a working and streamlined ITK will take some time to get right.

cquammen · 2016-08-29T03:01:58Z

FWIW, after exploring the use of ITK v4.9.0, v4.9.1, v4.10.0, and master (3174f0d5831870caf89af7f9c02a76d9287a60c1), the only version that I have gotten to build and work successfully with tomviz on Mac OS X is v4.9.0.

thewtex · 2016-08-29T03:18:40Z

@cquammen What issue came up with master?

cryos · 2016-08-29T11:40:57Z

@cquammen it would be great to consider whether these early branches can use the ITK we have in the superbuild (4.9.0 I think) with the same build options - this gives us a better chance of having nightly binaries that I can use at ToScA to show off new segmentation features.

Failing that, considering a minimal change from the build options we had would be great. I would like to avoid spending too much time changing ITK versions and/or build flags unless absolutely necessary. I would like to concentrate on the minimum needed to show viable ITK filters in Tomviz at this stage, and get the GUI elements in the application.

We will have time to go and revisit additional features we want/need from ITK once we have some of the support in the application.

cquammen · 2016-08-29T13:02:58Z

@cryos These changes were developed against the ITK v4.9.0 build in superbuild. They should work under any more recent version of ITK as well, aside from problems with the Python bindings I have run into. Please review under that assumption. I would like to get this merged today if possible to get the child data set work in place.

For posterity, my mistake was attempting to speed up the ITK build by turning off unneeded things, getting to an unusable build, then assuming any more recent version of ITK would be fine. BTW, I recommend avoiding pyenv Python versions on the Mac for building ParaView, ITK, etc. as ParaView fails when loading a VTK Python module. Don't know why.

cquammen · 2016-08-29T13:16:17Z

@thewtex I think this was my mistake. The error complains about compile-time numpy not matching runtime numpy.

cjh1 · 2016-08-29T13:16:47Z

@cquammen Yes I was able to get things up and running with the one change to the ITK build you recommended. Works goods and code looks good to me, I will let @cryos do the final sign off.

cquammen · 2016-08-29T13:25:21Z

Just to be clear, the current build instructions for ITK in https://github.com/OpenChemistry/tomviz/blob/master/BUILDING.md are valid. I originally told @cjh1 to modify an additional option to try to reduce the number of unneeded template instantiations without having tested it ahead of time.

cquammen · 2016-08-30T14:00:51Z

@cryos Are you happy with this change as is?

cryos · 2016-08-30T14:04:50Z

Yes, merging now, we can pick up any pieces during the week.

cryos · 2016-08-30T14:05:25Z

I was traveling/offline most of yesterday - that was the only reason for any delay @cquammen

cquammen · 2016-08-30T14:07:07Z

No problem. Thanks for reviewing and merging.

cquammen added the in progress label Aug 22, 2016

cquammen added 3 commits August 22, 2016 21:40

Added OperatorResult class

1e856c3

This class is a wrapper object for new data outputs produced by an Operator.

Add support for additional outputs produced by Operators

702d37d

This commit adds member functions to specify the number of artifacts an operator produces and to set those artifacts from VTK data objects.

Add display of operator results in the pipeline

0eb5794

cquammen force-pushed the operator_artifacts branch from c09ddf8 to 4b05e48 Compare August 23, 2016 02:54

cquammen changed the title ~~Operator artifacts~~ Operator results Aug 23, 2016

cquammen mentioned this pull request Aug 23, 2016

Compute some label object shape attributes #496

Closed

cquammen added 5 commits August 23, 2016 16:45

Refactor text file reading function

ff20d68

Added function to read in a text file with a given file root and an extension. Made existing readInPythonScript function use this function and added a readInJSONDescription function to load JSON text files.

Support setting of a JSON string in AddPythonTransformReaction

107cd0f

Use that support for loading the JSON file in the Connected Components filter.

Added prototype for JSON file describing Connected Components operator

c70836f

Add utils.make_spreadsheet() python function

53105a0

This is a handy function to create vtkTables from 2D numpy tables. Modified ConnectedComponents.py to use the function.

cjh1 reviewed Aug 25, 2016
View reviewed changes

Code style changes from clang-format

b1f1de4

cryos merged commit f3bd9fe into OpenChemistry:master Aug 30, 2016

cryos removed the in progress label Aug 30, 2016

cquammen deleted the operator_artifacts branch September 6, 2016 14:56

Operator results #567

Operator results #567

Conversation

cquammen commented Aug 22, 2016 • edited

cquammen commented Aug 22, 2016

Hovden commented Aug 22, 2016

cquammen commented Aug 22, 2016

cquammen commented Aug 22, 2016

Hovden commented Aug 22, 2016

cquammen commented Aug 23, 2016

cquammen commented Aug 23, 2016

cquammen commented Aug 23, 2016

cjh1 commented Aug 23, 2016

cjh1 commented Aug 23, 2016

cjh1 commented Aug 23, 2016

cquammen commented Aug 23, 2016

cquammen commented Aug 23, 2016

cjh1 commented Aug 23, 2016

mathturtle commented Aug 23, 2016

cjh1 commented Aug 23, 2016

cquammen commented Aug 23, 2016

cjh1 commented Aug 23, 2016

thewtex commented Aug 24, 2016

thewtex commented Aug 24, 2016

cquammen commented Aug 24, 2016

cjh1 commented Aug 25, 2016

cjh1 commented Aug 25, 2016

cjh1 Aug 25, 2016

Choose a reason for hiding this comment

cjh1 commented Aug 25, 2016 • edited

cquammen commented Aug 25, 2016

cryos commented Aug 25, 2016

cquammen commented Aug 25, 2016

cjh1 commented Aug 25, 2016 • edited

cjh1 commented Aug 25, 2016

cquammen commented Aug 26, 2016

cryos commented Aug 26, 2016

cquammen commented Aug 26, 2016

cquammen commented Aug 29, 2016

thewtex commented Aug 29, 2016

cryos commented Aug 29, 2016

cquammen commented Aug 29, 2016

cquammen commented Aug 29, 2016

cjh1 commented Aug 29, 2016

cquammen commented Aug 29, 2016

cquammen commented Aug 30, 2016

cryos commented Aug 30, 2016

cryos commented Aug 30, 2016

cquammen commented Aug 30, 2016

cquammen commented Aug 22, 2016 •

edited

cjh1 commented Aug 25, 2016 •

edited

cjh1 commented Aug 25, 2016 •

edited