[RFA/PATCH] FieldEngine FIXMEs anyone?

[Richard Guenther]

Hi!

While looking for the cause of centering views of FieldEngines being to
costly, I noticed several points in the FieldEngine implementation:
 - lots of FIXMEs!
 - a mix of weird centeringSize()/numMaterials() checks and
   implementations differing for specific cases
 - interesting way of keeping track of the domains in case of multiple
   centering points
 - everything is with respect to cell domains, not vertex domains (as
   arrays and also Meshes expect)

In the way of understanding whats going on, I added some documentation
and two new subfield view constructors (see patch below).

Can anyone elaborate on the successful usages of the centering/materials
concept from real applications?  Would it be acceptable to remove all the
special-cases for centeringSize()==1 and/or numMaterials()==1?

Does it really make sense to take views of multiple-centering fields? If
yes, can we change the expected domains to be vertex domains instead? So
an application can transparently switch Array/Field for vertex centerings.

Is there some documentation on why/how the INode and FieldEnginePatch
views are supposed to work and what is the advantage of using them
compared to usual domain views?


Thanks for any hints,

Richard.


===== FieldEngine.h 1.7 vs edited =====
--- 1.7/r2/src/Field/FieldEngine/FieldEngine.h  Mon Oct 27 11:25:16 2003
+++ edited/FieldEngine.h        Thu Nov  6 21:25:18 2003
@@ -37,7 +37,7 @@
 /** @file
  * @ingroup Field
  * @brief
- * FieldEngineBase and related classes.
+ * FieldEngine and FieldEngineBaseData classes.
  *
  * POOMA supports a flexible form
  * of "centering" that allows a hierarchy of multiple centering points per
@@ -69,6 +69,11 @@
 template<int Dim, class T, class EngineTag> class Engine;
 template<class Components> class ComponentWrapper;

+namespace Pooma {
+  struct MaterialViewTag {};
+  struct CenteringViewTag {};
+}
+

 /**
  * FieldEngineBaseData holds an engine and the relations.
@@ -83,6 +88,9 @@
     : engine_m()
   { }

+  /// Initializer to be used with an engine compatible layout or
+  /// similar initializer.
+
   template<class Initializer>
   FieldEngineBaseData(const Initializer &init)
     : engine_m(init)
@@ -192,41 +200,57 @@
   {
   }

-  /// Sub-field view constructor. This is when we want to construct a view of
-  /// one of the subFields in our top-level list.
-
-  FieldEngine(const This_t &model, int subField)
+  ///@name Sub-field view constructors
+  //@{
+
+  /// Takes a view of the specified centering point of the specified material.
+
+  FieldEngine(const This_t &model, int m, int c)
     : num_materials_m(1),
       stride_m(model.stride_m),
       physicalCellDomain_m(model.physicalCellDomain_m),
       guards_m(model.guards_m),
       mesh_m(model.mesh_m)
   {
-    if (model.numMaterials() > 1)
-    {
-      centering_m = model.centering();
-      data_m = model.data_m + model.stride_m * subField;
-    }
-    else
-    {
-      centering_m = model.centering()[subField];
-      data_m = model.data_m + subField;
-    }
+    PAssert((m >= 0) && (m < model.numMaterials()));
+    PAssert((c >= 0) && (c < model.centeringSize()));
+    centering_m = model.centering()[c];
+    data_m = model.data_m + model.stride_m * m + c;
   }

-  FieldEngine(const This_t &model, int m, int c)
-    : num_materials_m(1),
+  /// Takes a view of the specified centering point from all
+  /// materials.
+
+  FieldEngine(const This_t &model, int c, const Pooma::CenteringViewTag&)
+    : num_materials_m(model.num_materials_m),
       stride_m(model.stride_m),
       physicalCellDomain_m(model.physicalCellDomain_m),
       guards_m(model.guards_m),
       mesh_m(model.mesh_m)
   {
-    PAssert((m >= 0) && (m < model.numMaterials()));
     PAssert((c >= 0) && (c < model.centeringSize()));
     centering_m = model.centering()[c];
-    data_m = model.data_m + model.stride_m * m + c;
+    data_m = model.data_m + c;
   }

+  /// Takes a view of the specified material retaining all centering points.
+
+  FieldEngine(const This_t &model, int m, const Pooma::MaterialViewTag&)
+    : num_materials_m(1),
+      centering_m(model.centering_m),
+      stride_m(model.stride_m),
+      physicalCellDomain_m(model.physicalCellDomain_m),
+      guards_m(model.guards_m),
+      mesh_m(model.mesh_m)
+  {
+    PAssert((m >= 0) && (m < model.numMaterials()));
+    data_m = model.data_m + m * model.stride_m;
+  }
+
+  /// Takes a view of the specified centering point of the first material.
+  /// This is useless for fields with multiple materials and thus this
+  /// method is deprecated. Use FieldEngine(field, 0, c).
+
   FieldEngine(int c, const This_t &model)
     : num_materials_m(1),
       stride_m(model.stride_m),
@@ -239,7 +263,42 @@
     data_m = model.data_m + c;
   }

-  /// View constructors.
+  /// Takes a view of
+  ///  - the specified material including all centering points,
+  ///    if there is more than one material
+  ///  - the specified centering, if there is only one material
+  /// These are weird semantics and thus this method is deprecated.
+
+  FieldEngine(const This_t &model, int subField)
+    : num_materials_m(1),
+      stride_m(model.stride_m),
+      physicalCellDomain_m(model.physicalCellDomain_m),
+      guards_m(model.guards_m),
+      mesh_m(model.mesh_m)
+  {
+    PAssert((subField >= 0) && (subField < model.numSubFields()));
+    if (model.numMaterials() > 1)
+    {
+      centering_m = model.centering();
+      data_m = model.data_m + model.stride_m * subField;
+    }
+    else
+    {
+      centering_m = model.centering()[subField];
+      data_m = model.data_m + subField;
+    }
+  }
+
+  //@}
+
+  ///@name View constructors
+  //@{
+
+  /// Taking a domain view of the specified field-engine. For a field-engine
+  /// with one centering point and possibly multiple materials this is well
+  /// defined and expects the domain with respect to the centering of the
+  /// materials.  For multiple centerings this expects a cell domain as
+  /// viewing domain that is possibly extended to the respective vertex domain.

   template<class T2, class EngineTag2>
   FieldEngine(const FieldEngine<Mesh, T2, EngineTag2> &model,
@@ -277,29 +336,7 @@
     }
   }

-  /// This constructor handle weird things like range views.
-
-  template<class Mesh2, class T2, class EngineTag2, class Domain>
-  FieldEngine(const FieldEngine<Mesh2, T2, EngineTag2> &model,
-              const Domain &d)
-    : num_materials_m(model.numMaterials()),
-      centering_m(model.centering()),
-      stride_m(model.centeringSize()),
-      guards_m(0)
-  {
-    addSubFields();
-    // FIXME: Does this ever happen to fields with multiple centering points?
-    // (or event to fields with multiple materials???)
-    PAssert(model.centeringSize() == 1);
-    for (int m = 0; m < num_materials_m; ++m)
-    {
-      data(m, 0) = Data_t(model.data(m, 0).engine(), d,
-                          model.data(m, 0).relations());
-    }
-    // FIXME: how do we construct the mesh?????
-    mesh_m = 
Mesh(DomainLayout<Dim>(inputDomainToVertexDomain(data(0,0).engine().domain())));
-    physicalCellDomain_m = mesh_m.physicalCellDomain();
-  }
+  /// INode view.

   template<class T2, class EngineTag2>
   FieldEngine(const FieldEngine<Mesh, T2, EngineTag2> &model,
@@ -308,6 +345,11 @@
       centering_m(model.centering()),
       stride_m(model.centeringSize()),
       guards_m(0),
+      // FIXME: should hand INode to mesh?
+      // Probably, but Mesh(Mesh&, INode&) expects the INode domain
+      // being a vertex domain.
+      // This constructor expects a cell domain instead? I can't see how
+      // this can be correct for centering sizes > 1.
       mesh_m(model.mesh(),
              inputDomainToVertexDomain(i.domain())) // FIXME: should hand 
INode to mesh?
   {
@@ -363,6 +405,8 @@
     }
   }

+  /// FieldEnginePatch view.
+
   template<class EngineTag2>
   FieldEngine(const FieldEngine<Mesh, T, EngineTag2> &model,
               const FieldEnginePatch<Dim> &p)
@@ -370,7 +414,9 @@
       centering_m(model.centering()),
       stride_m(model.centeringSize()),
       guards_m(model.guardLayers()),
-      mesh_m(model.mesh()) // FIXME: should take a view of the mesh???
+      // FIXME: should take a view of the mesh???
+      // Yes. But again the mesh expects vertex domains.
+      mesh_m(model.mesh(), p)
   {
     // FIXME: should we copy the relations for patch?  Do we want
     // to take patch views of composite fields?
@@ -381,6 +427,8 @@
       centeringDomainToCellDomain(p.domain_m, centering_m, 0);
   }

+  /// Component view.
+
   template<class Mesh2, class T2, class EngineTag2, class Components>
   FieldEngine(const FieldEngine<Mesh2, T2, EngineTag2> &model,
               const ComponentWrapper<Components> &cw)
@@ -403,6 +451,33 @@
     }
   }

+  /// This constructor handle weird things like range views.
+  /// Its probably far from doing anything sane.
+
+  template<class Mesh2, class T2, class EngineTag2, class Domain>
+  FieldEngine(const FieldEngine<Mesh2, T2, EngineTag2> &model,
+              const Domain &d)
+    : num_materials_m(model.numMaterials()),
+      centering_m(model.centering()),
+      stride_m(model.centeringSize()),
+      guards_m(0)
+  {
+    addSubFields();
+    // FIXME: Does this ever happen to fields with multiple centering points?
+    // (or event to fields with multiple materials???)
+    PAssert(model.centeringSize() == 1);
+    for (int m = 0; m < num_materials_m; ++m)
+    {
+      data(m, 0) = Data_t(model.data(m, 0).engine(), d,
+                          model.data(m, 0).relations());
+    }
+    // FIXME: how do we construct the mesh?????
+    mesh_m = 
Mesh(DomainLayout<Dim>(inputDomainToVertexDomain(data(0,0).engine().domain())));
+    physicalCellDomain_m = mesh_m.physicalCellDomain();
+  }
+
+  //@}
+
   FieldEngine(const This_t &model,
               const Pooma::DontCopyRelations &d)
     : num_materials_m(model.numMaterials()),
@@ -673,6 +748,10 @@
   unsigned int num_materials_m;
   Centering<Dim> centering_m;
   int stride_m;
+  /// A one-dimensional organization of the FieldEngineBaseData
+  /// objects, for each of the numMaterials materials there are
+  /// centeringSize data objects. We use stride_m for taking
+  /// views, thus data(m,c) == data_m[material * stride_m + centering].
   RefCountedBlockPtr<Data_t> data_m;

   Domain_t physicalCellDomain_m;