ascent icon indicating copy to clipboard operation
ascent copied to clipboard

Rewrite libs/vtkh/vtkm_filters to use new vtk-m wrappers

Open nicolemarsaglia opened this issue 3 years ago • 18 comments

Some files in vtkm_filters point to worklets provided by vtk-m, this is not ideal since it bloats the build size for redundant code. VTK-m has gone through the process of providing wrappers for their filters that we should use instead.

nicolemarsaglia avatar Jul 27 '22 16:07 nicolemarsaglia

Issue isn't vtkm_filters. These are, for the most part, friendly wrappers that call vtkm functions that utilize vtkm worklets. With the exception of Threshold, which needs to have its worklet migrated to vtkm.

The majority of the problem children are in vtkh/filters. These directly utilize vtkm worklets and thus require the device compiler flags (this was a problem with amdgpu+kokkos and amr-wind and this will presumably be a fix?). Need to migrate these to vtkm, then write wrappers for the migrated worklets in vtkh/vtkm_filters, and then call wrapped functions in vtkh/filters. This is to follow the current format. Problem children: vtkh/vtkm_filters/threshold vtkh/filters/HistSampling vtkh/filters/Statistics vtkh/filters/CompositeVector vtkh/filters/Log vtkh/filters/GhostStripper vtkh/filters/Slice vtkh/filters/VectorMagnitude vtkh/filters/Clip vtkh/filters/Histogram

Note: vtkh is located in ascent/src/libs/

nicolemarsaglia avatar Aug 04 '22 19:08 nicolemarsaglia

  1. The worklet::CellDeepCopy in vtkh/vtkm_filters/threshold could be replaced by the CleanGrid filter.
  2. The vtkh/filter/Clip just added the MultiPlane ImplicitFunction to ClipWithImplicitFunction in a very ugly way. The Multplane ImplicitFunction could be added to vtk-m proper.
  3. vtkh/filter/Histogram should use vtkm/filter/density_estimate/Histogram as stated in the comment. This could be an exercise for Ascent developers.
  4. Same as vtkh/filter/VectorMagnitude.
  5. vtkh/filter/Log looks like an exercise/example. Do we really need it?
  6. vtkh/filter/CompositeVector could/should be replace by vtk-m's ArrayHandleComposite.
  7. vtkh/filter/Statistics should use the new vtkm/worklet/DescriptiveStatistics instead of calling Algorithm::Reduce etc.
  8. vtkh/filter/HistSampling should also be rewritten to use new random number generator and other new facilities in vtk-m.

ollielo avatar Aug 10 '22 20:08 ollielo

I was writing my response at the same time as @ollielo. Most of these are probably already implemented in VTK-m:

  • threshold: Are you referring to the use of vtkm/worklet/CellDeepCopy.h in vtkmThreshold.cpp? Using this should no longer be necessary. Just use vtkm::filter::entity_extraction::Threshold and you should be good.
  • GhostStripper: vtkm::filter::entity_extraction::GhostCellRemove
  • Slice: vtkm::filter::contour::Slice
  • VectorMagnitude: vtkm::filter::vector_analysis::VectorMagnitude
  • Clip: vtkm::filter::contour::ClipWithImplicitFunction
  • Histogram: vtkm::filter::density_estimate::Histogram

This just leaves:

  • HistSampling: I'm not sure what this does. Does it do a histogram of a random sample of the data? Is that really that much faster than making a single pass through the array?
  • Statistics: There is no filter that does statistics in VTK-m, but there is already some worklet code for it. I expect it to be pretty trivial to wrap that in a filter.
  • CompositeVector: I see why you would want this in a filter instead of using ArrayHandleComposite. A basic implementation should be pretty easy, but there are lots of special cases that would make this interesting.
  • Log: Trivial to implement (assuming you still want it).

kmorel avatar Aug 10 '22 20:08 kmorel

@kmorel

vtkm::filter::entity_extraction::Threshold doesn't do Compaction, it still returns a CellSetPermutation. If the goal is to remove/strip CellSetPermutation, a CleanGrid pass is still needed.

ollielo avatar Aug 10 '22 20:08 ollielo

HistSampling does random sampling according to some kind of probability density function (PDF). The point is to sample less from high probability region and more on the low-probability region (to maximize the entropy of the sample).

ollielo avatar Aug 10 '22 20:08 ollielo

Huh. I thought we changed that because a DataSet with. CellSetPermutation is all but useless. There should definitely be an option in Threshold (defaulted to true) that internally runs the result through CleanGrid.

kmorel avatar Aug 10 '22 20:08 kmorel

Adding CleanGrid to Threshold could also be an exercise for the new comer. There are plenty examples out there (e.g. ThresholdPoints and ExtracePoints).

ollielo avatar Aug 10 '22 20:08 ollielo

Correction, the Threshold does call CellDeepCopy deeply in its worklet:

https://gitlab.kitware.com/vtk/vtk-m/-/blob/master/vtkm/filter/entity_extraction/worklet/Threshold.h#L173

ollielo avatar Aug 11 '22 16:08 ollielo

Thanks. So it looks like Threshold is complete. Correct?

dpugmire avatar Aug 15 '22 14:08 dpugmire

Yes. The only thing you need to do is to remove the StripPermutation.

ollielo avatar Aug 15 '22 19:08 ollielo

After a short discussion with @dpugmire, we try to start with the Log filter. Firstly, we try to add a Log filter in the vtkm (assuming it is under the field_conversion dir, maybe we could set the base number as a parameter for the filter), then in the current vtkh log filter, we call that vtkm log filter directly. Does it makes sense for you?

Btw, should we implement CellLog filter and PointLog filter separately, or just put them together following the current vtkh example (such as this one https://github.com/Alpine-DAV/ascent/blob/develop/src/libs/vtkh/filters/Log.cpp#L459). Thanks!

wangzhezhe avatar Aug 15 '22 20:08 wangzhezhe

@wangzhezhe

It should be field_transform which transform fields from/to the same entity, instead of field_conversion which does something cell to point or point to cell conversion.

You really don't need separate CellLog and PointLog. The vtk-m infrastructure code can make one filter working for both cell and point field. I personally think it should also work for other field association (WHOLE_CELLSET) but it is up to you if you want it that way.

ollielo avatar Aug 15 '22 21:08 ollielo

BTW, the way that there are three filters doing essentially the same thing is not ideal. IMHO, the best way to correct it is something similar to the vtkm::ImplicitFunctionGeneral which uses Variant to select and hold different ImplicitFunctions.

ollielo avatar Aug 15 '22 21:08 ollielo

Thanks a lot for the information, it helps a lot ! @ollielo

wangzhezhe avatar Aug 15 '22 23:08 wangzhezhe

@dpugmire -- related to your question during telecon.

Here is the running list of VTK-h to VTK-m conversion details.

It's not 100% complete, but it's a good place to pick up the discussion.

cyrush avatar Dec 15 '22 22:12 cyrush

After several discussions with @nicolemarsaglia, we adapted the associated code from vtk-h and added several filters into vtk-m. Thanks a lot for the help from @kmorel, @dpugmire and @ollielo .

Log values https://gitlab.kitware.com/vtk/vtk-m/-/merge_requests/2852 Statistics filter https://gitlab.kitware.com/vtk/vtk-m/-/merge_requests/2957 Composite filter https://gitlab.kitware.com/vtk/vtk-m/-/merge_requests/2962 Distributed statistics filter https://gitlab.kitware.com/vtk/vtk-m/-/merge_requests/3017 Adaptive sampling https://gitlab.kitware.com/vtk/vtk-m/-/merge_requests/3083 MultiPlane https://gitlab.kitware.com/vtk/vtk-m/-/merge_requests/3124 SliceMultiple (3d slices) https://gitlab.kitware.com/vtk/vtk-m/-/merge_requests/3135

wangzhezhe avatar Oct 12 '23 18:10 wangzhezhe

Thanks @wangzhezhe for all your effort getting these filters rehomed, and for making this list!

I'm going to keep this issue open until I've fully finished Ascent's implementation.

nicolemarsaglia avatar Oct 13 '23 22:10 nicolemarsaglia

Thanks for the help! @nicolemarsaglia, just let me know if there are anything that I could help with!

wangzhezhe avatar Oct 13 '23 23:10 wangzhezhe