Merge topic 'field-length-checking'

63702d5d1 Check to make sure that the fields in a DataSet are the proper length

Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: Li-Ta Lo <ollie@lanl.gov>
Merge-request: !2914

docs/changelog/field-length-checking.md

@@ -0,0 +1,12 @@
+# Check to make sure that the fields in a DataSet are the proper length
+
+It is possible in a `DataSet` to add a point field (or coordinate system)
+that has a different number of points than reported in the cell set.
+Likewise for the number of cells in cell fields. This is very bad practice
+because it is likely to lead to crashes in worklets that are expecting
+arrays of an appropriate length.
+
+Although `DataSet` will still allow this, a warning will be added to the
+VTK-m logging to alert users of the inconsistency introduced into the
+`DataSet`. Since warnings are by default printed to standard error, users
+are likely to see it.

vtkm/cont/DataSet.cxx

@@ -9,6 +9,57 @@
 //============================================================================
 
 #include <vtkm/cont/DataSet.h>
+#include <vtkm/cont/Logging.h>
+
+namespace
+{
+
+VTKM_CONT void CheckFieldSize(const vtkm::cont::UnknownCellSet& cellSet,
+                              const vtkm::cont::Field& field)
+{
+  if (!cellSet.IsValid())
+  {
+    return;
+  }
+  switch (field.GetAssociation())
+  {
+    case vtkm::cont::Field::Association::Points:
+      if (cellSet.GetNumberOfPoints() != field.GetData().GetNumberOfValues())
+      {
+        VTKM_LOG_S(vtkm::cont::LogLevel::Warn,
+                   "The size of field `"
+                     << field.GetName() << "` (" << field.GetData().GetNumberOfValues()
+                     << " values) does not match the size of the data set structure ("
+                     << cellSet.GetNumberOfPoints() << " points).");
+      }
+      break;
+    case vtkm::cont::Field::Association::Cells:
+      if (cellSet.GetNumberOfCells() != field.GetData().GetNumberOfValues())
+      {
+        VTKM_LOG_S(vtkm::cont::LogLevel::Warn,
+                   "The size of field `"
+                     << field.GetName() << "` (" << field.GetData().GetNumberOfValues()
+                     << " values) does not match the size of the data set structure ("
+                     << cellSet.GetNumberOfCells() << " cells).");
+      }
+      break;
+    default:
+      // Ignore as the association does not match any topological element.
+      break;
+  }
+}
+
+VTKM_CONT void CheckFieldSizes(const vtkm::cont::UnknownCellSet& cellSet,
+                               const vtkm::cont::internal::FieldCollection& fields)
+{
+  vtkm::IdComponent numFields = fields.GetNumberOfFields();
+  for (vtkm::IdComponent fieldIndex = 0; fieldIndex < numFields; ++fieldIndex)
+  {
+    CheckFieldSize(cellSet, fields.GetField(fieldIndex));
+  }
+}
+
+} // anonymous namespace
 
 namespace vtkm
 {
@@ -38,6 +89,12 @@ void DataSet::Clear()
   this->CellSet = this->CellSet.NewInstance();
 }
 
+void DataSet::AddField(const Field& field)
+{
+  CheckFieldSize(this->CellSet, field);
+  this->Fields.AddField(field);
+}
+
 vtkm::Id DataSet::GetNumberOfCells() const
 {
   return this->CellSet.GetNumberOfCells();
@@ -45,11 +102,43 @@ vtkm::Id DataSet::GetNumberOfCells() const
 
 vtkm::Id DataSet::GetNumberOfPoints() const
 {
-  if (this->CoordSystems.empty())
+  if (this->CellSet.IsValid())
   {
-    return 0;
+    return this->CellSet.GetNumberOfPoints();
   }
-  return this->CoordSystems[0].GetNumberOfPoints();
+
+  // If there is no cell set, then try to use a coordinate system to get the number
+  // of points.
+  if (this->GetNumberOfCoordinateSystems() > 0)
+  {
+    return this->GetCoordinateSystem().GetNumberOfPoints();
+  }
+
+  // If there is no coordinate system either, we can try to guess the number of
+  // points by finding a point field.
+  for (vtkm::IdComponent fieldIdx = 0; fieldIdx < this->Fields.GetNumberOfFields(); ++fieldIdx)
+  {
+    const vtkm::cont::Field& field = this->Fields.GetField(fieldIdx);
+    if (field.GetAssociation() == vtkm::cont::Field::Association::Points)
+    {
+      return field.GetData().GetNumberOfValues();
+    }
+  }
+
+  // There are no point fields either.
+  return 0;
+}
+
+void DataSet::AddCoordinateSystem(const vtkm::cont::CoordinateSystem& cs)
+{
+  CheckFieldSize(this->CellSet, cs);
+  this->CoordSystems.push_back(cs);
+}
+
+void DataSet::SetCellSetImpl(const vtkm::cont::UnknownCellSet& cellSet)
+{
+  CheckFieldSizes(cellSet, this->Fields);
+  this->CellSet = cellSet;
 }
 
 void DataSet::CopyStructure(const vtkm::cont::DataSet& source)
@@ -57,6 +146,8 @@ void DataSet::CopyStructure(const vtkm::cont::DataSet& source)
   this->CoordSystems = source.CoordSystems;
   this->CellSet = source.CellSet;
   this->GhostCellName = source.GhostCellName;
+
+  CheckFieldSizes(this->CellSet, this->Fields);
 }
 
 const vtkm::cont::CoordinateSystem& DataSet::GetCoordinateSystem(vtkm::Id index) const

vtkm/cont/DataSet.h

@@ -55,7 +55,7 @@ public:
   /// to have the same number of points.
   VTKM_CONT vtkm::Id GetNumberOfPoints() const;
 
-  VTKM_CONT void AddField(const Field& field) { this->Fields.AddField(field); }
+  VTKM_CONT void AddField(const Field& field);
 
   VTKM_CONT
   const vtkm::cont::Field& GetField(vtkm::Id index) const { return this->Fields.GetField(index); }
@@ -265,10 +265,7 @@ public:
 
 
   VTKM_CONT
-  void AddCoordinateSystem(const vtkm::cont::CoordinateSystem& cs)
-  {
-    this->CoordSystems.push_back(cs);
-  }
+  void AddCoordinateSystem(const vtkm::cont::CoordinateSystem& cs);
 
   VTKM_CONT
   bool HasCoordinateSystem(const std::string& name) const
@@ -310,7 +307,7 @@ public:
   VTKM_CONT void SetCellSet(const CellSetType& cellSet)
   {
     VTKM_IS_KNOWN_OR_UNKNOWN_CELL_SET(CellSetType);
-    this->CellSet = vtkm::cont::UnknownCellSet(cellSet);
+    this->SetCellSetImpl(cellSet);
   }
 
   VTKM_CONT
@@ -359,6 +356,8 @@ private:
 
   vtkm::cont::UnknownCellSet CellSet;
   std::shared_ptr<std::string> GhostCellName;
+
+  VTKM_CONT void SetCellSetImpl(const vtkm::cont::UnknownCellSet& cellSet);
 };
 
 } // namespace cont

vtkm/cont/testing/UnitTestDataSetConvertToExpected.cxx

@@ -11,6 +11,7 @@
 #include <vtkm/cont/ArrayCopy.h>
 #include <vtkm/cont/ArrayHandleUniformPointCoordinates.h>
 #include <vtkm/cont/CastAndCall.h>
+#include <vtkm/cont/CellSetStructured.h>
 #include <vtkm/cont/DataSet.h>
 #include <vtkm/cont/Field.h>
 
@@ -61,6 +62,10 @@ vtkm::cont::DataSet MakeDataSet()
 
   vtkm::cont::DataSet dataset;
 
+  vtkm::cont::CellSetStructured<3> cellSet;
+  cellSet.SetPointDimensions(vtkm::Id3(DIM_SIZE));
+  dataset.SetCellSet(cellSet);
+
   dataset.AddCoordinateSystem(vtkm::cont::CoordinateSystem("coords", MakeCoordinates<FieldType>()));
   dataset.AddPointField("scalars", MakeField<FieldType>());
   dataset.AddPointField("vectors", MakeVecField<FieldType>());

vtkm/cont/testlib/MakeTestDataSet.cxx

@@ -1302,7 +1302,7 @@ vtkm::cont::DataSet MakeTestDataSet::Make3DExplicitDataSet8()
 
   // Coordinates
   const int nVerts = 8;
-  const int nCells = 10;
+  const int nCells = 9;
   using CoordType = vtkm::Vec3f_32;
   std::vector<CoordType> coords = { { -0.707f, -0.354f, -0.354f }, { 0.000f, -0.854f, 0.146f },
                                     { 0.000f, -0.146f, 0.854f },   { -0.707f, 0.354f, 0.354f },
@@ -1366,7 +1366,7 @@ vtkm::cont::DataSet MakeTestDataSet::Make3DExplicitDataSet8()
 
   // Field data
   vtkm::Float32 pointvar[nVerts] = { 100.0f, 78.0f, 49.0f, 17.0f, 94.0f, 71.0f, 47.0f, 57.0f };
-  vtkm::Float32 cellvar[nCells] = { 0.0f, 1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f, 7.0f, 8.0f, 9.0f };
+  vtkm::Float32 cellvar[nCells] = { 0.0f, 1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f, 7.0f, 8.0f };
 
   dataSet.AddPointField("pointvar", pointvar, nVerts);
   dataSet.AddCellField("cellvar", cellvar, nCells);