DofManager cleanup #1412

klevzoff · 2021-05-03T09:07:07Z

Reduce the number of memory accesses when populating dof index arrays by slightly tweaking the numbering logic
Replace mesh visitor raw loops with RAJA loops and use parallel host policy where appropriate (e.g. when counting)
Remove concatenated region names from dof index wrapper name - it is no longer useful when each DofManager instance has its own unique name prefix.
Add ComponentMask abstraction that replaces the [loComp; hiComp) pairs in methods that do component sub-setting (all vector-to-field and field-to-vector methods, as well as makeRestrictor), which allows for more flexible specification of component mapping (previously only a contiguous range). It acts like a simple dynamic-size bitset of fixed capacity (up to 64, so that it fits into a single unsigned 64-bit int), but is host-device marked so it can be used in device kernels.
Remove a bunch of methods related to sparsity pattern generation on LAI matrices - we don't use that capability anymore outside of unit tests but it contained a lot of duplicate code.
Remove the artificial limit on number of fields in DofManager by changing the storage for coupling data from array2d to map.
Removed multiple overloads of addField and addCoupling methods (seems easy enough to just provide all parameters at all call sites)
Miscellaneous minor code cleanups

No change in functionality otherwise, but rebaseline required to a slight naming scheme change for dof index arrays - even though dof index array have a NO_WRITE flag, the wrapper name still appears (without data) in the restart file.

src/coreComponents/linearAlgebra/utilities/ComponentMask.hpp

klevzoff · 2021-05-07T00:07:15Z

@rrsettgast @joshua-white @castelletto1 @andrea-franceschini I don't know who to ask for a review, since nobody really looks into the guts of DofManager much I think, and this is mostly just code cleanup. Feel free to (not) leave a review.

rrsettgast

I still think we should have a common set of looping patterns between the physics solvers and the DOFmanager. I think my previous proposal to move these out of the DofManager must have fallen flat. Perhaps we can develop a separate entity that will hold all the looping patterns for both the DOFManager and the physics solvers to use.

rrsettgast · 2021-05-07T23:13:05Z

src/cmake/GeosxConfig.cmake

+    set( GEOSX_LA_INTERFACE "Hypre" )
+    set( GEOSX_LA_INTERFACE_HYPRE ON )
+    set( GEOSX_LA_INTERFACE_TRILINOS OFF )
+    set( GEOSX_LA_INTERFACE_PETSC OFF )


Aren't these the defaults in geosxoptions.cmake?

They are, but they're overridden by user host-config. One of the recent PRs actually committed changes to doxygen/GeosxConfig.hpp, reverting it to Trilinos once again, because we were missing enforcement of these LAI parameters here.

rrsettgast · 2021-05-07T23:15:00Z

src/coreComponents/physicsSolvers/simplePDE/PhaseFieldDamageFEM.cpp

+  dofManager.addField( m_fieldName,
+                       DofManager::Location::Node,
+                       1,
+                       targetRegionNames() );


Is targetRegionNames() used if the DOF is on a node?

Yes. DOF index will only be assigned to nodes that are within or adjacent to target regions (although the index array obviously is allocated on all nodes).

rrsettgast · 2021-05-11T16:26:09Z

src/coreComponents/mesh/utilities/MeshMapUtilities.hpp

+/**
+ * @brief @return the size of the map along first dimension
+ * @tparam T type of map element
+ * @param map reference to the map
+ */
+template< typename T >
+inline localIndex size0( arrayView1d< arrayView1d< T const > const > const & map )
+{
+  return map.size();
+}
+
+/**
+ * @copydoc size0(arrayView1d< arrayView1d< T const > const > const &)
+ * @tparam USD unit-stride dimension of the map
+ */
+template< typename T, int USD >
+inline localIndex size0( arrayView2d< T, USD > const & map )
+{
+  return map.size( 0 );
+}
+
+/**
+ * @copydoc size0(arrayView1d< arrayView1d< T const > const > const &)
+ */
+template< typename T >
+inline localIndex size0( ArrayOfArraysView< T const > const & map )
+{
+  return map.size();
+}
+
+/**
+ * @copydoc size0(arrayView1d< arrayView1d< T const > const > const &)
+ */
+template< typename T >
+inline localIndex size0( ArrayOfSetsView< T const > const & map )
+{
+  return map.size();
+}


I think these could be a single function if you wanted

Between arrayView1d< arrayView1d>, ArrayOfSetsView and ArrayOfArraysView for sure, but arrayView2d is different (size() with no arguments does not do what I want). I chose to explicitly spell out all supported types here rather than make a template function to handle 3 cases together (I didn't even need T parameter here tbh, since mesh maps are always localIndex). Or did you have something else in mind?

rrsettgast · 2021-05-11T18:05:30Z

src/coreComponents/linearAlgebra/utilities/ComponentMask.hpp

+  v--;
+  v |= v >> 1;
+  v |= v >> 2;
+  v |= v >> 4;
+  v |= v >> 8;
+  v |= v >> 16;
+  v++;
+  return v;


does v need to be unsigned for this to always work?

Yes, will fix

rrsettgast · 2021-05-11T18:08:24Z

src/coreComponents/linearAlgebra/DofManager.hpp

                    arrayView1d< string const > const & regions,
-                    bool const symmetric );
+                    bool symmetric = true );


Would it be better or worse to not specify a default here?

I think true is a reasonable default that is used close to 100% of the time. If one way coupling is assumed, don't people tend to do operator splitting instead of a monolithic matrix approach?

klevzoff · 2021-05-12T06:17:07Z

I still think we should have a common set of looping patterns between the physics solvers and the DOFmanager. I think my previous proposal to move these out of the DofManager must have fallen flat. Perhaps we can develop a separate entity that will hold all the looping patterns for both the DOFManager and the physics solvers to use.

As far as sparsity generation is concerned, we're sort of moving towards having these looping patterns abstracted similar to current FEM kernel hierarchy. We'll hopefully have something similar for stencil-based FV, and then all sparsity-related stuff will be removed from DofManager. I don't have the time for that refactoring just yet, but it's on the radar. There will always be extra complexity though (e.g. needing to reach multiple levels of mesh adjacency, like nodes of elements connected though a stencil entry) that is intimately related to the details of specific physics/discretization. Not sure how hard/easy it is go generalize.

The other use of mesh looping here is enumerating dofs, which entails visiting each locally owned mesh object (node, face, element) within target regions exactly once. This is not unlike looping over m_targetNodes in SolidMechanicsLagrangianFEM (except that DofManager doesn't persist the node set but rebuilds it every time), so there's definitely some overlap here, but there are subtle details like the split into sendOrReceiveNodes vs nonSendOrReceiveNodes that solvers need. We could design some common MeshVisitor object that abstracts all of that from solvers, but I'd like to get sparsity patterns handled first.

TotoGaz

Some missed the action comment!

TotoGaz · 2021-05-20T18:35:59Z

src/coreComponents/linearAlgebra/interfaces/hypre/HypreMGRStrategies.hpp

@@ -52,7 +52,7 @@ namespace mgr
 */
 template< typename STRATEGY >
 void setStrategy( LinearSolverParameters::MGR const & params,
-                  arrayView1d< localIndex const > const & numComponentsPerField,
+                  arrayView1d< integer const > const & numComponentsPerField,


Hi @klevzoff !

I notice that you change localIndex to integer in your patch.
I always thought that behind the naming localIndex and globalIndex there was some kind of idea that those indices we intrinsically not the same kind of indices: some local to the MPI rank, some global to the simulation.

In some sense, it was like we were using some Hungarian notation for indices. (Not exactly since we use it on types and not on variable names, but there is some flavor).

(It's true that here numComponentsPerField is not indeed an index.)

We had spoken about having some unit system inside of GEOSX.
Do you think this could also be a use case for this unit stuff?

poke @rrsettgast

Hi @TotoGaz ! What you describe for localIndex vs globalIndex is true. Using either of these types to represent number of components per degree-of-freedom (or component index for that matter) doesn't feel right, because it doesn't scale with number of mesh objects (either local or global), even though it often gets added to those things to form a new (local or global) index. It was a historical mistake of me misusing localIndex for all kinds of things where I shouldn't have.

Doing what you suggest in my mind implies introducing strong types for indices, and while it's not a bad idea per se, I imagine this being a huge headache in terms of implementation, especially considering that index arrays are part of interop with TPLs (e.g. directly handed off to linear algebra packages).

Doing what you suggest in my mind implies introducing strong types for indices, and while it's not a bad idea per se, I imagine this being a huge headache in terms of implementation, especially considering that index arrays are part of interop with TPLs (e.g. directly handed off to linear algebra packages).

Indeed that may be an issue.
There was this idea to typedef our type to unit<int> or int based on some compilation flag.
Maybe there are some unit libs that allow some automatic conversion from their unit<int> to int (maybe an option? Maybe units::no_dim<int> -> int ?). But what about pointers to data ? This needs to be investigated as part of the more global discussion on units.

TotoGaz · 2021-05-20T18:46:27Z

src/coreComponents/linearAlgebra/utilities/LAIHelperFunctions.hpp

@@ -247,7 +247,7 @@ void computeRigidBodyModes( MeshLevel const & mesh,
  array1d< globalIndex > globalNodeList;
  for( localIndex k = 0; k < LvArray::integerConversion< localIndex >( selection.size() ); ++k )
  {
-    if( dofManager.getLocation( selection[k] ) == DofManager::Location::Node )
+    if( dofManager.location( selection[k] ) == DofManager::Location::Node )


The problem with this

Suggested change

if( dofManager.location( selection[k] ) == DofManager::Location::Node )

if( dofManager.getLocation( selection[k] ) == DofManager::Location::Node )

is that with the get prefix, without knowing what I am talking about, I know that I am probably accessing some immutable component (or something like that) of location.
Without get, it's not obvious anymore if I am setting location to a certain value and then use some return type while passing through. It's an additional cost for new comers.

In this case I just made it consistent with other member functions that didn't use get prefix (numComponents(), rankOffset(), etc.). I also just realized there is still one inconsistent member getKey() left, but... "we'll get'em next time".

In general, I prefer thing() and setThing() for getter and setter, respectively. I think any mutating member function should contain an action verb, and a lack thereof signifies a "passive" getter or some kind of const inspection method. This seems to me like a pretty intuitive convention (though exceptions are possible, of course).

(For the record, I totally used to be in the get/set camp. My first OOP language was Java in ~2006, and this convention was prevalent there at the time. I think I shifted over time, especially as I looked more and more at modern C++ code bases and style guides).

OK then!

Personally I am in the "think less" side. get, set and we're all set 😉. And also on rule with no exception: one function gets a verb and that's it.
Also, when in the IDE, you can just type get and ask for completion. That's really convenient when you do not know where you are.

klevzoff added type: cleanup / refactor Non-functional change (NFC) flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests labels May 3, 2021

klevzoff self-assigned this May 3, 2021

klevzoff force-pushed the feature/klevzoff/dof-manager-cleanup branch 6 times, most recently from d896158 to 4795a33 Compare May 6, 2021 20:13

klevzoff commented May 7, 2021

View reviewed changes

src/coreComponents/linearAlgebra/utilities/ComponentMask.hpp Outdated Show resolved Hide resolved

klevzoff requested review from rrsettgast, andrea-franceschini, castelletto1 and joshua-white May 7, 2021 00:05

rrsettgast approved these changes May 11, 2021

View reviewed changes

DofManager cleanup

54d8855

klevzoff force-pushed the feature/klevzoff/dof-manager-cleanup branch from 4795a33 to 54d8855 Compare May 12, 2021 07:14

Update baselines

031bb3d

klevzoff mentioned this pull request May 12, 2021

[Bug] OSX CI broken again #1423

Closed

Disable unit testing in OSX

e2a406a

klevzoff added ci: run CUDA builds Allows to triggers (costly) CUDA jobs and removed flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests labels May 12, 2021

klevzoff merged commit e98d043 into develop May 13, 2021

klevzoff deleted the feature/klevzoff/dof-manager-cleanup branch May 13, 2021 00:25

TotoGaz reviewed May 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DofManager cleanup #1412

DofManager cleanup #1412

klevzoff commented May 3, 2021 •

edited

Loading

klevzoff commented May 7, 2021

rrsettgast left a comment

rrsettgast May 7, 2021

klevzoff May 12, 2021

rrsettgast May 7, 2021

klevzoff May 12, 2021

rrsettgast May 11, 2021

klevzoff May 12, 2021

rrsettgast May 11, 2021

klevzoff May 12, 2021

rrsettgast May 11, 2021

klevzoff May 12, 2021

klevzoff commented May 12, 2021

TotoGaz left a comment

TotoGaz May 20, 2021

klevzoff May 20, 2021

TotoGaz May 20, 2021

TotoGaz May 20, 2021

klevzoff May 20, 2021

TotoGaz May 20, 2021 •

edited

Loading

	if( dofManager.location( selection[k] ) == DofManager::Location::Node )
	if( dofManager.getLocation( selection[k] ) == DofManager::Location::Node )

DofManager cleanup #1412

DofManager cleanup #1412

Conversation

klevzoff commented May 3, 2021 • edited Loading

klevzoff commented May 7, 2021

rrsettgast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klevzoff commented May 12, 2021

TotoGaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotoGaz May 20, 2021 • edited Loading

Choose a reason for hiding this comment

klevzoff commented May 3, 2021 •

edited

Loading

TotoGaz May 20, 2021 •

edited

Loading