Merge topic 'ghost-cell-remove-interface'

a62eb7eb8 Update the interface and documentation of GhostCellRemove

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !3110

docs/changelog/ghost-cell-remove-interface.md

@@ -0,0 +1,10 @@
+# Updated the interface and documentation of GhostCellRemove
+
+The `GhostCellRemove` filter had some methods inconsistent with the naming
+convention elsewhere in VTK-m. The class itself was also in need of some
+updated documentation. Both of these issues have been fixed.
+
+Additionally, there were some conditions that could lead to unexpected
+behavior. For example, if the filter was asked to remove only ghost cells
+and a cell was both a ghost cell and blank, it would not be removed. This
+has been updated to be more consistent with expectations.

vtkm/filter/entity_extraction/GhostCellRemove.cxx

@@ -19,37 +19,33 @@
 
 namespace
 {
-class RemoveAllGhosts
+
+template <typename T>
+VTKM_EXEC inline bool ShouldRemove(T value, vtkm::UInt8 removeTypes)
+{
+  return ((value & removeTypes) != 0);
+}
+
+class RemoveGhostPredicate
 {
 public:
-  VTKM_CONT
-  RemoveAllGhosts() = default;
-
-  VTKM_EXEC bool operator()(const vtkm::UInt8& value) const { return (value == 0); }
-};
-
-class RemoveGhostByType
-{
-public:
-  VTKM_CONT
-  RemoveGhostByType()
-    : RemoveType(0)
+  VTKM_CONT RemoveGhostPredicate()
+    : RemoveTypes(0xFF)
   {
   }
 
-  VTKM_CONT
-  explicit RemoveGhostByType(const vtkm::UInt8& val)
-    : RemoveType(static_cast<vtkm::UInt8>(~val))
+  VTKM_CONT explicit RemoveGhostPredicate(vtkm::UInt8 val)
+    : RemoveTypes(val)
   {
   }
 
   VTKM_EXEC bool operator()(const vtkm::UInt8& value) const
   {
-    return value == 0 || (value & RemoveType);
+    return !ShouldRemove(value, this->RemoveTypes);
   }
 
 private:
-  vtkm::UInt8 RemoveType;
+  vtkm::UInt8 RemoveTypes;
 };
 
 template <int DIMS>
@@ -87,10 +83,9 @@ class RealMinMax : public vtkm::worklet::WorkletMapField
 {
 public:
   VTKM_CONT
-  RealMinMax(vtkm::Id3 cellDims, bool removeAllGhost, vtkm::UInt8 removeType)
+  RealMinMax(vtkm::Id3 cellDims, vtkm::UInt8 removeTypes)
     : CellDims(cellDims)
-    , RemoveAllGhost(removeAllGhost)
-    , RemoveType(removeType)
+    , RemoveTypes(removeTypes)
   {
   }
 
@@ -121,8 +116,10 @@ public:
   VTKM_EXEC void operator()(const T& value, const vtkm::Id& index, AtomicType& atom) const
   {
     // we are finding the logical min max of valid cells
-    if ((RemoveAllGhost && value != 0) || (!RemoveAllGhost && (value != 0 && value | RemoveType)))
+    if (ShouldRemove(value, this->RemoveTypes))
+    {
       return;
+    }
 
     vtkm::Id3 logical = getLogical<DIMS>(index, CellDims);
 
@@ -137,8 +134,7 @@ public:
 
 private:
   vtkm::Id3 CellDims;
-  bool RemoveAllGhost;
-  vtkm::UInt8 RemoveType;
+  vtkm::UInt8 RemoveTypes;
 };
 
 template <int DIMS>
@@ -166,13 +162,9 @@ class Validate : public vtkm::worklet::WorkletMapField
 {
 public:
   VTKM_CONT
-  Validate(const vtkm::Id3& cellDims,
-           bool removeAllGhost,
-           vtkm::UInt8 removeType,
-           const vtkm::RangeId3& range)
+  Validate(const vtkm::Id3& cellDims, vtkm::UInt8 removeTypes, const vtkm::RangeId3& range)
     : CellDims(cellDims)
-    , RemoveAll(removeAllGhost)
-    , RemoveVal(removeType)
+    , RemoveVals(removeTypes)
     , Range(range)
   {
   }
@@ -181,33 +173,31 @@ public:
   typedef void ExecutionSignature(_1, InputIndex, _2);
 
   template <typename T>
-  VTKM_EXEC void operator()(const T& value, const vtkm::Id& index, vtkm::UInt8& valid) const
+  VTKM_EXEC void operator()(const T& value, const vtkm::Id& index, vtkm::UInt8& invalid) const
   {
-    valid = 0;
-    if (RemoveAll && value == 0)
-      return;
-    else if (!RemoveAll && (value == 0 || (value & RemoveVal)))
-      return;
-
-    if (checkRange<DIMS>(Range, getLogical<DIMS>(index, CellDims)))
-      valid = static_cast<vtkm::UInt8>(1);
+    if (ShouldRemove(value, this->RemoveVals) &&
+        checkRange<DIMS>(Range, getLogical<DIMS>(index, CellDims)))
+    {
+      invalid = static_cast<vtkm::UInt8>(1);
+    }
+    else
+    {
+      invalid = 0;
+    }
   }
 
 private:
   vtkm::Id3 CellDims;
-  bool RemoveAll;
-  vtkm::UInt8 RemoveVal;
+  vtkm::UInt8 RemoveVals;
   vtkm::RangeId3 Range;
 };
 
 template <int DIMS, typename T, typename StorageType>
 bool CanStrip(const vtkm::cont::ArrayHandle<T, StorageType>& ghostField,
               const vtkm::cont::Invoker& invoke,
-              bool removeAllGhost,
-              vtkm::UInt8 removeType,
+              vtkm::UInt8 removeTypes,
               vtkm::RangeId3& range,
-              const vtkm::Id3& cellDims,
-              vtkm::Id size)
+              const vtkm::Id3& cellDims)
 {
   vtkm::cont::ArrayHandle<vtkm::Id> minmax;
   minmax.Allocate(6);
@@ -218,18 +208,17 @@ bool CanStrip(const vtkm::cont::ArrayHandle<T, StorageType>& ghostField,
   minmax.WritePortal().Set(4, std::numeric_limits<vtkm::Id>::min());
   minmax.WritePortal().Set(5, std::numeric_limits<vtkm::Id>::min());
 
-  invoke(RealMinMax<3>(cellDims, removeAllGhost, removeType), ghostField, minmax);
+  invoke(RealMinMax<3>(cellDims, removeTypes), ghostField, minmax);
 
   auto portal = minmax.ReadPortal();
   range = vtkm::RangeId3(
     portal.Get(0), portal.Get(3), portal.Get(1), portal.Get(4), portal.Get(2), portal.Get(5));
 
-  vtkm::cont::ArrayHandle<vtkm::UInt8> validFlags;
-  validFlags.Allocate(size);
+  vtkm::cont::ArrayHandle<vtkm::UInt8> invalidFlags;
 
-  invoke(Validate<DIMS>(cellDims, removeAllGhost, removeType, range), ghostField, validFlags);
+  invoke(Validate<DIMS>(cellDims, removeTypes, range), ghostField, invalidFlags);
 
-  vtkm::UInt8 res = vtkm::cont::Algorithm::Reduce(validFlags, vtkm::UInt8(0), vtkm::Maximum());
+  vtkm::UInt8 res = vtkm::cont::Algorithm::Reduce(invalidFlags, vtkm::UInt8(0), vtkm::Maximum());
   return res == 0;
 }
 
@@ -237,8 +226,7 @@ template <typename T, typename StorageType>
 bool CanDoStructuredStrip(const vtkm::cont::UnknownCellSet& cells,
                           const vtkm::cont::ArrayHandle<T, StorageType>& ghostField,
                           const vtkm::cont::Invoker& invoke,
-                          bool removeAllGhost,
-                          vtkm::UInt8 removeType,
+                          vtkm::UInt8 removeTypes,
                           vtkm::RangeId3& range)
 {
   bool canDo = false;
@@ -249,9 +237,8 @@ bool CanDoStructuredStrip(const vtkm::cont::UnknownCellSet& cells,
     auto cells1D = cells.AsCellSet<vtkm::cont::CellSetStructured<1>>();
     vtkm::Id d = cells1D.GetCellDimensions();
     cellDims[0] = d;
-    vtkm::Id sz = d;
-
-    canDo = CanStrip<1>(ghostField, invoke, removeAllGhost, removeType, range, cellDims, sz);
+    VTKM_ASSERT(ghostField.GetNumberOfValues() == cellDims[0]);
+    canDo = CanStrip<1>(ghostField, invoke, removeTypes, range, cellDims);
   }
   else if (cells.CanConvert<vtkm::cont::CellSetStructured<2>>())
   {
@@ -259,15 +246,15 @@ bool CanDoStructuredStrip(const vtkm::cont::UnknownCellSet& cells,
     vtkm::Id2 d = cells2D.GetCellDimensions();
     cellDims[0] = d[0];
     cellDims[1] = d[1];
-    vtkm::Id sz = cellDims[0] * cellDims[1];
-    canDo = CanStrip<2>(ghostField, invoke, removeAllGhost, removeType, range, cellDims, sz);
+    VTKM_ASSERT(ghostField.GetNumberOfValues() == (cellDims[0] * cellDims[1]));
+    canDo = CanStrip<2>(ghostField, invoke, removeTypes, range, cellDims);
   }
   else if (cells.CanConvert<vtkm::cont::CellSetStructured<3>>())
   {
     auto cells3D = cells.AsCellSet<vtkm::cont::CellSetStructured<3>>();
     cellDims = cells3D.GetCellDimensions();
-    vtkm::Id sz = cellDims[0] * cellDims[1] * cellDims[2];
-    canDo = CanStrip<3>(ghostField, invoke, removeAllGhost, removeType, range, cellDims, sz);
+    VTKM_ASSERT(ghostField.GetNumberOfValues() == (cellDims[0] * cellDims[1] * cellDims[2]));
+    canDo = CanStrip<3>(ghostField, invoke, removeTypes, range, cellDims);
   }
 
   return canDo;
@@ -330,8 +317,7 @@ VTKM_CONT vtkm::cont::DataSet GhostCellRemove::DoExecute(const vtkm::cont::DataS
       cells.CanConvert<vtkm::cont::CellSetStructured<3>>())
   {
     vtkm::RangeId3 range;
-    if (CanDoStructuredStrip(
-          cells, fieldArray, this->Invoke, this->GetRemoveAllGhost(), this->GetRemoveType(), range))
+    if (CanDoStructuredStrip(cells, fieldArray, this->Invoke, this->GetTypesToRemove(), range))
     {
       vtkm::filter::entity_extraction::ExtractStructured extract;
       extract.SetInvoker(this->Invoke);
@@ -352,19 +338,8 @@ VTKM_CONT vtkm::cont::DataSet GhostCellRemove::DoExecute(const vtkm::cont::DataS
   vtkm::cont::UnknownCellSet cellOut;
   vtkm::worklet::Threshold worklet;
 
-  if (this->GetRemoveAllGhost())
-  {
-    cellOut = worklet.Run(cells, fieldArray, field.GetAssociation(), RemoveAllGhosts());
-  }
-  else if (this->GetRemoveByType())
-  {
-    cellOut = worklet.Run(
-      cells, fieldArray, field.GetAssociation(), RemoveGhostByType(this->GetRemoveType()));
-  }
-  else
-  {
-    throw vtkm::cont::ErrorFilterExecution("Unsupported ghost cell removal type");
-  }
+  cellOut = worklet.Run(
+    cells, fieldArray, field.GetAssociation(), RemoveGhostPredicate(this->GetTypesToRemove()));
 
   auto mapper = [&](auto& result, const auto& f) { DoMapField(result, f, worklet); };
   return this->CreateResult(input, cellOut, mapper);

vtkm/filter/entity_extraction/GhostCellRemove.h

@@ -12,6 +12,7 @@
 #define vtk_m_filter_entity_extraction_GhostCellRemove_h
 
 #include <vtkm/CellClassification.h>
+#include <vtkm/Deprecated.h>
 #include <vtkm/filter/FilterField.h>
 #include <vtkm/filter/entity_extraction/vtkm_filter_entity_extraction_export.h>
 
@@ -21,47 +22,81 @@ namespace filter
 {
 namespace entity_extraction
 {
-/// \brief Removes ghost cells
+/// \brief Removes cells marked as ghost cells.
+///
+/// This filter inspects the ghost cell field of the input and removes any cells
+/// marked as ghost cells. Although this filter nominally operates on ghost cells,
+/// other classifications, such as blanked cells, can also be recorded in the ghost
+/// cell array. See `vtkm::CellClassification` for the list of flags typical in a
+/// ghost array.
+///
+/// By default, if the input is a structured data set the filter will attempt to
+/// output a structured data set. This will be the case if all the cells along a
+/// boundary are marked as ghost cells together, which is common. If creating a
+/// structured data set is not possible, an explicit data set is produced.
 ///
 class VTKM_FILTER_ENTITY_EXTRACTION_EXPORT GhostCellRemove : public vtkm::filter::FilterField
 {
 public:
-  VTKM_CONT
-  GhostCellRemove();
+  VTKM_CONT GhostCellRemove();
 
-  VTKM_CONT
-  void RemoveGhostField() { this->RemoveField = true; }
-  VTKM_CONT
-  void RemoveAllGhost() { this->RemoveAll = true; }
-  VTKM_CONT
-  void RemoveByType(const vtkm::UInt8& vals)
-  {
-    this->RemoveAll = false;
-    this->RemoveVals = vals;
-  }
-  VTKM_CONT
-  bool GetRemoveGhostField() const { return this->RemoveField; }
-  VTKM_CONT
-  bool GetRemoveAllGhost() const { return this->RemoveAll; }
+  /// @brief Specify whether the ghost cell array should be removed from the input.
+  ///
+  /// If this flag is true, then the ghost cell array will not be
+  /// passed to the output.
+  VTKM_CONT void SetRemoveGhostField(bool flag) { this->RemoveField = flag; }
+  /// @copydoc SetRemoveGhostField
+  VTKM_CONT bool GetRemoveGhostField() const { return this->RemoveField; }
 
-  VTKM_CONT
-  bool GetUseGhostCellsAsField() const { return this->UseGhostCellsAsField; }
-  VTKM_CONT
-  void SetUseGhostCellsAsField(bool flag) { this->UseGhostCellsAsField = flag; }
+  /// @brief Specify which types of cells to remove.
+  ///
+  /// The types to remove are specified by the flags in `vtkm::CellClassification`.
+  /// Any cell with a ghost array flag matching one or more of these flags will be removed.
+  VTKM_CONT void SetTypesToRemove(vtkm::UInt8 typeFlags) { this->TypesToRemove = typeFlags; }
+  /// @copydoc SetTypesToRemove
+  VTKM_CONT vtkm::UInt8 GetTypesToRemove() const { return this->TypesToRemove; }
 
-  VTKM_CONT
-  bool GetRemoveByType() const { return !this->RemoveAll; }
-  VTKM_CONT
-  vtkm::UInt8 GetRemoveType() const { return this->RemoveVals; }
+  /// @brief Set filter to remove any special cell type.
+  ///
+  /// This method sets the state to remove any cell that does not have a "normal" ghost
+  /// cell value of 0. Any other value represents a cell that is placeholder or otherwise
+  /// not really considered part of the cell set.
+  VTKM_CONT void SetTypesToRemoveToAll() { this->SetTypesToRemove(0xFF); }
+  /// @brief Returns true if all abnormal cell types are removed.
+  VTKM_CONT bool AreAllTypesRemoved() const { return this->GetTypesToRemove() == 0xFF; }
+
+  VTKM_DEPRECATED(2.1, "Use SetRemoveGhostField(true).")
+  VTKM_CONT void RemoveGhostField() { this->SetRemoveGhostField(true); }
+  VTKM_DEPRECATED(2.1, "Use SetTypesToRemoveToAll().")
+  VTKM_CONT void RemoveAllGhost() { this->SetTypesToRemoveToAll(); }
+  VTKM_DEPRECATED(2.1, "Use SetTypesToRemove(vals).")
+  VTKM_CONT void RemoveByType(const vtkm::UInt8& vals) { this->SetTypesToRemove(vals); }
+
+  VTKM_DEPRECATED(2.1, "Use AreAllTypesRemoved().")
+  VTKM_CONT bool GetRemoveAllGhost() const { return this->AreAllTypesRemoved(); }
+
+  /// @brief Specify whether the marked ghost cells or a named field should be used as the ghost field.
+  ///
+  /// When this flag is true (the default), the filter will get from the input
+  /// `vtkm::cont::DataSet` the field (with the `GetGhostCellField` method). When
+  /// this flag is false, the `SetActiveField` method of this class should be used
+  /// to select which field to use as ghost cells.
+  VTKM_CONT bool GetUseGhostCellsAsField() const { return this->UseGhostCellsAsField; }
+  /// @copydoc GetUseGhostCellsAsField
+  VTKM_CONT void SetUseGhostCellsAsField(bool flag) { this->UseGhostCellsAsField = flag; }
+
+  VTKM_DEPRECATED(2.1, "Use !AreAllTypesRemoved().")
+  VTKM_CONT bool GetRemoveByType() const { return !this->AreAllTypesRemoved(); }
+  VTKM_DEPRECATED(2.1, "Use GetTypesToRemove().")
+  VTKM_CONT vtkm::UInt8 GetRemoveType() const { return this->GetTypesToRemove(); }
 
 private:
   VTKM_CONT
   vtkm::cont::DataSet DoExecute(const vtkm::cont::DataSet& input) override;
 
   bool UseGhostCellsAsField = true;
-  bool RemoveAll = false;
   bool RemoveField = false;
-  vtkm::UInt8 RemoveVals = 0;
+  vtkm::UInt8 TypesToRemove = 0xFF;
 };
 
 } // namespace entity_extraction

vtkm/filter/entity_extraction/testing/UnitTestGhostCellRemove.cxx

@@ -276,12 +276,12 @@ void TestGhostCellRemove()
           for (auto& rt : removeType)
           {
             vtkm::filter::entity_extraction::GhostCellRemove ghostCellRemoval;
-            ghostCellRemoval.RemoveGhostField();
+            ghostCellRemoval.SetRemoveGhostField(true);
 
             if (rt == "all")
-              ghostCellRemoval.RemoveAllGhost();
+              ghostCellRemoval.SetTypesToRemoveToAll();
             else if (rt == "byType")
-              ghostCellRemoval.RemoveByType(vtkm::CellClassification::Ghost);
+              ghostCellRemoval.SetTypesToRemove(vtkm::CellClassification::Ghost);
 
             auto output = ghostCellRemoval.Execute(ds);
             vtkm::Id numCells = output.GetNumberOfCells();
@@ -323,7 +323,7 @@ void TestGhostCellRemove()
               ds = MakeRectilinear(nx, ny, nz, layer, nameType, true);
 
             vtkm::filter::entity_extraction::GhostCellRemove ghostCellRemoval;
-            ghostCellRemoval.RemoveGhostField();
+            ghostCellRemoval.SetRemoveGhostField(true);
             auto output = ghostCellRemoval.Execute(ds);
             VTKM_TEST_ASSERT(output.GetCellSet().IsType<vtkm::cont::CellSetExplicit<>>(),
                              "Wrong cell type for explicit conversion");