Interface and naming conventions

[barker29]

The names of objects, methods, and so forth are not always consistent throughout the code. This issue is intended to collect some of the known inconsistencies and discuss how the code could be changed or documented to remove or mitigate some of these inconsistencies.

• Element: spelled "Element", "Elem", or "E" at various places. Note also that this word has two semantic meanings as well, as a finite element or as an element of a Vector/Matrix.
• Number: spelled "Number", "Num", or "N" at various places.
• Parallel: spelled "Parallel" or "Par" variously in the code.
• Dimension: spelled "Dimension" (as in Mesh) or "Dim" (as in FiniteElement)
• Communicator: spelled "Comm" almost everywhere but in GroupCommunicator
• Boundary: spelled "Boundary", "Bdr", or "B", this one is an inconsistent mess throughout the code.
• Column: spelled "Column" (as in SparseMatrix) or "Col" (as in HypreParMatrix)
• ElementVector/SubVector: in the Vector class, we have AddElementVector but GetSubVector and SetSubVector.

[barker29]

Regarding "Parallel":

In my view "Par" is much more common in the code than "Parallel", so I would recommend:

• Within ParGridFunction, rename ParallelAverage(), ParallelAssemble(), and ParallelProject()
• Within ParLinearForm, rename ParallelAssemble()
• Within ParBilinearForm, rename ParallelAssemble(), ParallelAssembleElim(), ParallelEliminateEssentialBC(), ParallelEliminateTDofs()
• Within StaticCondensation, rename GetParallelMatrix()

[barker29]

Regarding "Number":

"N" is used very often but is probably not always clear to users, and I would recommend replacing it with "Num":

• In Mesh: GetNV, GetNE, GetNBE, GetNEdges, GetGlobalNE, GetNFaces (note that GetNumFaces() already exists and appears to do something different from GetNFaces!)
• in 'FiniteElementSpace`: GetNDofs, GetNConformingDofs, GetNVDofs, GetNEDofs, GetNFDofs, GetNF, GetNRanks
• in StaticCondensation: GetNPrDofs, GetNExDofs
• in Table: NumberOfRows, NumberOfEntries could be spelled "Num" for consistency

[v-dobrev]

One thing that will definitely help is to document what the methods do, so there is no room for misinterpretation, if you read the documentation.

One additional item I can add:

• Face: it can mean two different things: a 2D mesh entity, or (dim-1)-dimensional mesh entity in a dim-dimensional mesh.

Having clear conventions for naming things is great and can help a lot but one still needs to learn and remember what they are. Documentation is where one can learn these.

Even in the current state, there is some logic and conventions used for naming things, even if they are not fully consistent. For example, based on the context, I don't think using one or the other of the following is really confusing: Num vs Number, Dim vs Dimension, Col vs Column, etc.

[rw-anderson]

When you are looking at Num vs Number, I agree that there is no real ambiguity. It's more a matter of when you want to invoke one of those methods, and you don't remember which way it is named, so you have to go look it up each time. It is a minor thing, but nice to have. Whether it is worth breaking old code to change it? I don't know.

[v-dobrev]

If we start renaming things, I think we should definitely keep the old names too, at least for some time.

[termi-official]

Hi @rw-anderson ,
in internal projects I tackled such issues gradually. First, marking the problematic functions and methods as deprecated, either through function attributes (__attribute__, __declspec, ...) or the deprecated specifier). This way users have time to modernize their code. Then, with the next major version bump, the deprecated functions were removed, complemented with a changelog to give users a better chance to modernize old snippets, which may reside in gists, solved issues, blog posts, ....
I really second documenting these kinds of problems, but in general I do not think is worth breaking interfaces too quickly (as @v-dobrev also said in the meantime).

So, here is my round of semantic problems which I would like to address in this thread.

1. const correctness, especially with respect to getter methods.
   Marking immutable data "const" possibly unlocks code optimization and really helps in writing programs with shared memory parallelism (e.g. through OpenMP), because data races are impossible. For example, the following example
   

   mfem/mesh/mesh.cpp

   Lines 415 to 419 in 657c64a

   	ElementTransformation *Mesh::GetBdrElementTransformation(int i)
   	{
   	GetBdrElementTransformation(i, &BdrTransformation);
   	return &BdrTransformation;
   	}

   
   can lead to really hard to track down issues, because multiple threads reference to the same mutable object and may change the integration point simultaneously. On the other hand the following method (called by the previous one)
   

   mfem/mesh/mesh.cpp

   Line 421 in 657c64a

   void Mesh::GetBdrElementTransformation(int i, IsoparametricTransformation* ElTr)

   
   seems to be somewhat const correct.
   I also see from time to time getter functions which either do a substatial amount of computationas internally (e.g. the error estimators) or which transfer ownership (i.e. create objects which the caller must cleanup).
   

   mfem/fem/pfespace.hpp

   Lines 284 to 287 in 657c64a

   	/** @brief For a non-conforming mesh, construct and return the interpolation
   	matrix from the partially conforming true dofs to the local dofs. */
   	/** @note The returned pointer must be deleted by the caller. */
   	HypreParMatrix *GetPartialConformingInterpolation();

   
   While this is documented, this still can become an issue, as it is not the behavior I would expect from a method starting with "get".
   Note that marking a method (or data) const is potentially a breaking change for user code.
2. ownership and parameter passing
   It is often not directly clear when to pass a pointer or a reference and its consequence to the objects ownership. While in most places the data members are documented, methods do not reflect this. E.g. in
   

   mfem/fem/fespace.hpp

   Lines 296 to 302 in c9d97af

   	FiniteElementSpace(const FiniteElementSpace &orig, Mesh *mesh = NULL,
   	const FiniteElementCollection *fec = NULL);
   	FiniteElementSpace(Mesh *mesh,
   	const FiniteElementCollection *fec,
   	int vdim = 1, int ordering = Ordering::byNODES)
   	{ Constructor(mesh, NULL, fec, vdim, ordering); }

   
   the mesh ownership is not transfered, while e.g.
   

   mfem/fem/nonlinearform.hpp

   Lines 84 to 86 in 657c64a

   	/// Adds new Domain Integrator.
   	void AddDomainIntegrator(NonlinearFormIntegrator *nlfi)
   	{ dnfi.Append(nlfi); }

   
   does. Especially the latter one gave me a really hard time to track down in my AMR simulations.
   Here a good example is the design of the ZZ error estimator construction
   

   mfem/fem/estimators.hpp

   Lines 111 to 119 in ca5a4df

   	/** @brief Construct a new ZienkiewiczZhuEstimator object.
   	@param integ This BilinearFormIntegrator must implement the methods
   	ComputeElementFlux() and ComputeFluxEnergy().
   	@param sol The solution field whose error is to be estimated.
   	@param flux_fes The ZienkiewiczZhuEstimator assumes ownership of this
   	FiniteElementSpace and will call its Update() method when
   	needed.*/
   	ZienkiewiczZhuEstimator(BilinearFormIntegrator &integ, GridFunction &sol,
   	FiniteElementSpace *flux_fes)

   
   

   mfem/fem/estimators.hpp

   Lines 131 to 139 in ca5a4df

   	/** @brief Construct a new ZienkiewiczZhuEstimator object.
   	@param integ This BilinearFormIntegrator must implement the methods
   	ComputeElementFlux() and ComputeFluxEnergy().
   	@param sol The solution field whose error is to be estimated.
   	@param flux_fes The ZienkiewiczZhuEstimator does NOT assume ownership of
   	this FiniteElementSpace; will call its Update() method
   	when needed. */
   	ZienkiewiczZhuEstimator(BilinearFormIntegrator &integ, GridFunction &sol,
   	FiniteElementSpace &flux_fes)

   
   because references for non-owned inputs and pointers for taking ownership is a widespread convention.
3. access and indexing of non-local quantities
   This one is from my view as a developer, when trying to extend MFEM. For me it is still a point of confusion whenever I have to use/pass negative indices (starting at -2!) and when to use "max-local-quantity + non-local-index". E.g. FaceInfo sometimes stores negative indices in Face1Inf and Face2Inf. I am not sure what exactly Inf stands for and what "Inf = 64 * LocalFaceIndex + FaceOrientation " tells me. On the other hand ParGridFunction::GetValue allows me to access grid data of adjacent processes (after calling ParGridFunction::ExchangeFaceNbrData), via an index larger than the number of local dofs. Also, vdim starts at 1 and not 0 (?) if I understand correctly.
   Again Doxygen misses to capture some documentation for FaceInfo (the note below the class), which I think is quite essential to grasp what is going on. I also noticed that there is no way of accessing the NCInfo, which is (may be?) necessary to distinguish whether a face is a slave or master face.

Since I think this thread is also an oppurtunity to improve documentation in general, here two more points which catched my attention

1. working with non-conforming meshes
   I figured out by try-and-error, that sub-face integration (i.e. the integral over individual "slave-parts") works out of the box with the FaceElementTransformations class. I could not find a place where this is documented. Also, it is unclear to me why FaceElementTransformations::face is deprecated, as it is quite useful, especially when working with a distributed memory application (because it gives direct access to the correct shared face). Also, Doxygen misses the documentation for this class.
2. Involved operators and the different vectorial quantities
   When I started using mfem it was really not clear how exactly the operator decomposition (i.e. https://mfem.org/performance/ and https://ceed.exascaleproject.org/libceed/) relates to which classes in the code and how to access these quantities/operators. I am not sure about this one, but probably small code snippets could help (new) users here.

Btw, for documentation which must be shared between functions and methods, Doxygen has a feature called grouping, which may become handy. Especially since the classes have quite some methods and finding the "right" ones can take some time and often methods share some common documentation (and context).

[rw-anderson]

"Inf = 64 * LocalFaceIndex + FaceOrientation"

You don't find raw bit shifting and packing intuitively obvious?
Yeah, it wasn't to me either. 😄

[barker29]

@termi-official Thanks for your very detailed comments! Honestly I did not intend this issue to be quite so broad when I opened it - I am wondering if things like const-correctness and ownership of pointers should be documented and discussed in separate issues?

[thorvath12]

If we start renaming things, I think we should definitely keep the old names too, at least for some time.

Keeping the old names is a very good idea. However, I have a question that is more or less related to this. I noticed that there are 2 functions AddTriangle and AddTia that are seemingly the same. Is there any preference to use one over the other? Was it that someone wanted to change the name, and now we have both for backward compatibility?

Tamas

[v-dobrev]

I noticed that there are 2 functions AddTriangle and AddTia that are seemingly the same. Is there any preference to use one over the other? Was it that someone wanted to change the name, and now we have both for backward compatibility?

These two are exactly the same -- just alternative names.

I had to check the 1-line implementations to see that, since we the only "documentation" for these is that they are part of the "documentation group" that starts here:

mfem/mesh/mesh.hpp

Lines 503 to 507 in 3c5dc27

	/** @name Methods for Mesh construction.
	These methods are intended to be used with the @ref mfem_Mesh_init_ctor
	"init constructor". */
	///@{

[termi-official]

@barker29 In case the question was directed at me, I would keep it in this thread. Simply because the mentioned contracts (i.e. pre-, postconditions, invariants and possibly local or global side effects) are part of an interface and its conventions.

[thorvath12]

I had to check the 1-line implementations to see that, since we the only "documentation" for these is that they are part of the "documentation group" that starts here:

mfem/mesh/mesh.hpp

Lines 503 to 507 in 3c5dc27

	/** @name Methods for Mesh construction.
	These methods are intended to be used with the @ref mfem_Mesh_init_ctor
	"init constructor". */
	///@{

Yes, I saw that the implementations are the same. That's why I thought that one of those will be deleted at some point and that could cause a problem. (The same problem will happen if you rename anything that is suggested in this issue and delete the old names anytime in the future.)

[rw-anderson]

If this issue is for general interface issues, I thought I'd raise the issue of the "Coefficient" classes which I presume were originally used to describe coefficients in PDEs, but seem to now function as more of a general non-FE function interface that is used for other things like defining initial conditions, exact solutions, due to the convenience of being able to project into a FE space.

I wonder if something as simple as a typedef to allow the alias "Function" in place of "Coefficient" would make programmer intent a bit less confusing in places.

[termi-official]

I found another small one in the Mesh regarding the connectivity information, which is at least not obvious to me.
There is
const Table & ElementToElementTable ()
without const
const Table & ElementToFaceTable () const + const Table & | ElementToEdgeTable () const
with const and further
Table *GetFaceEdgeTable () const + Table * GetEdgeVertexTable () const
withouth the "to" and a mutable Table instance.

Going through the implementation I guess the latter two are specific to 3D meshes while the former are for "ND" meshes? It also looks like the Mesh has the ownership of the latter?

EDIT: Forgot Table * mfem::Mesh::GetVertexToElementTable( ) and Table * mfem::Mesh::GetFaceToElementTable ( ) const, which transfer ownership to the caller and the former seem to have the const missing?

[stale]

⚠️ This issue or PR has been automatically marked as stale because it has not had any activity in the last month. If no activity occurs in the next week, it will be automatically closed. Thank you for your contributions.

[barker29]

Bumping because I think this is a useful discussion, even if most of this is not likely to be fixed soon.

[pazner]

Another source of inconsistencies is whether accessors should include the prefix Get. For example, Vector and Array provide GetData() functions. DenseMatrix provides both Data() and GetData() (they do the same thing, should one be deprecated?). DenseTensor provides GetData(int) and Data() (these are two versions of related accessors — why should the semantics depend on the prefix Get?).

Similarly, to get the mesh from a finite element space, you use GetMesh, however to get the finite element space from a grid function you use FESpace().

ParFiniteElementSpace has both GetGroupComm and GroupComm member functions, which do very different things (one is private, but the point still stands...).

[rw-anderson]

Are Mesh.Print() and GridFunction.Save() named differently for a reason? They seem to be very similar.

[v-dobrev]

I think Save is to distinguish it from the Print inherited from Vector. The former calls the latter.

[FunMiles]

I have just started working with MFEM and as I was looking at GetBdrElementTransformation and was surprised it was not marked as const. Searching for const correctness in the issue brought me here. As @termi-official noted, more methods should be made const. I have a few more comments on his remarks:

1. const correctness, especially with respect to getter methods.
   Marking immutable data "const" possibly unlocks code optimization and really helps in writing programs with shared memory parallelism (e.g. through OpenMP), because data races are impossible.

I think const is mostly not there for the compiler but for the programmers. It helps a reader better understand the code, for example immediately seeing when an argument being passed is possibly an output rather than just an input data. Having non const-correct code means in some places, a pointer or reference argument cannot be marked as const as well and the reader either is likely to make an incorrect assumption or has to open the code to find what is going on.

For example, the following example

mfem/mesh/mesh.cpp

Lines 415 to 419 in 657c64a

	ElementTransformation *Mesh::GetBdrElementTransformation(int i)
	{
	GetBdrElementTransformation(i, &BdrTransformation);
	return &BdrTransformation;
	}

can lead to really hard to track down issues, because multiple threads reference to the same mutable object and may change the integration point simultaneously. On the other hand the following method (called by the previous one)

mfem/mesh/mesh.cpp

Line 421 in 657c64a

void Mesh::GetBdrElementTransformation(int i, IsoparametricTransformation* ElTr)

seems to be somewhat const correct.

Actually this function, though it should be const cannot be marked as const because it uses some internal temporary data space (FaceElemTr for 2D periodic meshes).
It seems that the use of temporary internal data space variable in the class Mesh is imho too widespread. Several methods return pointers to internal data space, including the one argument GetBdrElementTransformation mentioned by @termi-official. Modern allocators/deallocators are fast and C++11 has added move semantics to avoid copies when returning dynamically allocated values. It seems that in modern C++11, values should be returned from such one-argument function.

[stale]

⚠️ This issue or PR has been automatically marked as stale because it has not had any activity in the last month. If no activity occurs in the next week, it will be automatically closed. Thank you for your contributions.

[rw-anderson]

Has it ever been considered to use a naming convention to distinguish member variables? Like google's convention to use a trailing underscore? I find this helpful when trying to understand complex classes. I recognize this would be a sweeping change.

[tzanio]

Has it ever been considered to use a naming convention to distinguish member variables? Like google's convention to use a trailing underscore? I find this helpful when trying to understand complex classes. I recognize this would be a sweeping change.

It could be something to consider for mfem-5.0

[rw-anderson]

Has it ever been considered to use a naming convention to distinguish member variables? Like google's convention to use a trailing underscore? I find this helpful when trying to understand complex classes. I recognize this would be a sweeping change.

It could be something to consider for mfem-5.0

Ok. Only if there is broad support for it. I know opinions vary on this.

[v-dobrev]

In Qt Creator (and maybe other editors), you can configure it to show member variables with a different style -- e.g. I configured it to show then in italic.

That said, I don't see a problem in using some convention for that. A common choice is to use the prefix m_.

[rw-anderson]

In Qt Creator (and maybe other editors), you can configure it to show member variables with a different style -- e.g. I configured it to show then in italic.

Interesting. Does Qt Creator parse the code with a C++ compiler to determine that?

That said, I don't see a problem in using some convention for that. A common choice is to use the prefix m_.

m_ is what I am most accustomed to. I have been told recently by younger developers that this is considered archaic. But I personally would be comfortable with it.

[v-dobrev]

Interesting. Does Qt Creator parse the code with a C++ compiler to determine that?

Yes, it uses a version of clang that comes with it.

[termi-official]

Found some issues related to the matrix infrastructure. Since this is giving me constant issues in my code I am wondering why PetscParMatrix and HypreParMatrix just specialized on Operator and not on AbstractSparseMatrix - is there some rationale to not do this?

Further matrix patching/elimination methods are inconsistent and DiagonalPolicy seems to be not really used.

In the HypreParMatrix some time the eliminated entries are returned, but no option how the diagonal should be handled or docs on what happens to the entry.

HypreParMatrix * HypreParMatrix::EliminateRowsCols (const Array< int > &rows_cols)
HypreParMatrix * HypreParMatrix::EliminateCols (const Array< int > &cols)
void HypreParMatrix::EliminateRows (const Array< int > &rows)

Here the elimination methods have even different semantics. The first method eliminates the rows/cols of the input matrix and assigns itself the eliminated entries, which is not true for the latter two methods. Again no way to handle the diagonal entries.

void OperatorHandle::EliminateRowsCols (OperatorHandle &A, const Array< int > &ess_dof_list)
void OperatorHandle::EliminateRows (const Array< int > &ess_dof_list)
void OperatorHandle::EliminateCols (const Array< int > &ess_dof_list)

Here we the EliminateCols method is missing completely. Also, row elimination does not give an option to handle the diagonal entry or docs on what happens to the entry.

void PetscParMatrix::EliminateRowsCols (const Array< int > &rows_cols, const PetscParVector &X, PetscParVector &B, double diag=1.)
void PetscParMatrix::EliminateRowsCols (const Array< int > &rows_cols, const HypreParVector &X, HypreParVector &B, double diag=1.)
PetscParMatrix * PetscParMatrix::EliminateRowsCols (const Array< int > &rows_cols)
void PetscParMatrix::EliminateRows (const Array< int > &rows)

Not quite sure about the "vanilla" one.

void SparseMatrix::EliminateRow (int row, const double sol, Vector &rhs)
void SparseMatrix::EliminateRow (int row, DiagonalPolicy dpolicy=DIAG_ZERO)
void SparseMatrix::EliminateCol (int col, DiagonalPolicy dpolicy=DIAG_ZERO)
void SparseMatrix::EliminateCols (const Array< int > &cols, const Vector *x=NULL, Vector *b=NULL)
void SparseMatrix::EliminateCols (const Array< int > &col_marker, SparseMatrix &Ae)
void SparseMatrix::EliminateRowCol (int rc, const double sol, Vector &rhs, DiagonalPolicy dpolicy=DIAG_ONE)
void SparseMatrix::EliminateRowCol (int rc, DiagonalPolicy dpolicy=DIAG_ONE)
void SparseMatrix::EliminateRowCol (int rc, SparseMatrix &Ae, DiagonalPolicy dpolicy=DIAG_ONE)
void SparseMatrix::EliminateRowColDiag (int rc, double value)