Trilinos: Tpetra: FECrsMatrix fill/resume inconsistency

[bartgol]

Bug Report

@trilinos/tpetra

Description

I'm trying to use the (relatively) new FECrsMatrix, and I'm getting an error that I think is due to the way the class handles the inactive matrix during beginFill/endFill calls.

I am recycling the same matrix multiple times, so I perform multiple beginFill/endFill calls. This yields an error during endFill, which spits out the CrsMatrix error:

Throw test that evaluated to true: (! this->isFillActive () || this->isFillComplete ())

Tpetra::CrsMatrix<...>::fillComplete: Matrix fill state must be active (isFillActive() must be true) before you may call fillComplete().

I think the problem is in the beginFill method. The resumeFill method from CrsMatrix is only called on the current object, not on inactiveCrsMatrix_. However, during endFill, fillComplete is called on both matrices. This results in an error on the "owned" matrix (the one without the shared dofs).

I tried the following:

@@ -132,6 +132,7 @@ void FECrsMatrix<Scalar, LocalOrdinal, GlobalOrdinal, Node>::beginFill()  {
   // Note: This does not throw an error since the on construction, the FECRS is in overlap mode.  Ergo, calling beginFill(),
   // like one should expect to do in a rational universe, should not cause an error.
   if(*activeCrsMatrix_ == FE_ACTIVE_OWNED) {
+    this->resumeFill();
     switchActiveCrsMatrix();
   }
   this->resumeFill();

and the error went away.

I want to point out that this is mostly a matter of 'inconsistency' (an asymmetry between begin and end of fill), rather than a real 'bug', but the inconsistency may be confusing. One could still get the correct result by manually calling resumeFill (on the FE matrix in OWNED state) _before_ calling beginFill. However, this is confusing when one reads the code, since the endFill call is not followed by a fillComplete call (since that's done automatically in endFill).

My suggestion is to call resumeFill also on the owned matrix, during a call to beginFill.

Steps to Reproduce

1. SHA1: 27d6e354a4
2. Create a FECrsMatrix m, using 2+ ranks.
3. call, in sequence, m.endFill(); m.beginFill(); m.endFill(). The last one should throw.
4. Shake fist angrily at computer.

[csiefer2]

@bartgol Yup. What you suggest is the behavior we want.

[csiefer2]

Fix merge. @bartgol please close issue if the fix meets your needs.

[bartgol]

Great, thanks for doing this so quickly!