Convert FunctionBCs to FunctorBCs accepting Coefficient inputs formed from Functions or Materials

[alexanderianblair]

Following on from the discussion in #61 and #53 regarding a single store of mfem::Coefficients, mfem::VectorCoefficients, and mfem::MatrixCoefficients to enable Functor-like functionality in Platypus using coefficients, this PR converts existing FunctionBCs and kernels to Functor equivalents that can take a Coefficient type in their input, repurposing the PropertyManager to act as the problem Coefficient store.

In addition, some minor readability updates to handling of Coefficient names has also been carried out for improved clarity when referring to different Coefficient types in parameter declarations.

[codecov]

Codecov Report

Attention: Patch coverage is 99.77195% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/userobjects/MFEMGeneralUserObject.C	66.66%	2 Missing ⚠️

Files with missing lines	Coverage Δ
src/bcs/MFEMConvectiveHeatFluxBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMScalarDirichletBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMScalarFunctorBoundaryIntegratedBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMScalarFunctorDirichletBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMVectorBoundaryIntegratedBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMVectorDirichletBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMVectorDirichletBCBase.C	100.00% <100.00%> (ø)
src/bcs/MFEMVectorFunctorBoundaryIntegratedBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMVectorFunctorDirichletBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMVectorFunctorDirichletBCBase.C	100.00% <100.00%> (ø)
... and 39 more

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[cmacmackin]

I've made a number of initial comments/asked some questions. A few thoughts:

• I'd really prefer if we could redo the file name changes so they were accomplished with git mv, as this makes changes to those files much easier to review.
• We should be more consistent in using the terms "property", "coefficient", and "functor".
  • Property should be reserved for things associated with materials
  • We need to choose whether to use functor or coefficient for the rest
• I think the materials should subclass FunctorMaterial
• How much do we want to imitate the MOOSE functor access interface?
  • We could just make coefficients be analagous to functors. You can use them regardless of whether they are defined by material properties, functions, auxiliary variables, post-processors, etc., but the functions for declaring and accessing them have completely different interfaces and names.
  • We could try to mimic functors as much as possible, including using the name.
• Should we add the ability to access coefficients for variables, auxvariables, and post-processors, as can be done for functors?

I think we should change the naming convention for the declareXProperty methods. Methods that take a list block IDs will keep their names. Those that do not will be called declareXCoefficient instead. This makes it clear that properties are a way of building up piecewise data, associated with particular parts of the mesh that correspond to a material, while coefficients are a more generic concept. Incidentally, this will avoid some overloading and simplify the interface for the variadic templated methods.

This isn't meant to be a final review; there may well be something I'm forgetting or missed.

[cmacmackin]

I've handled the comments I raised on particular parts of the code. Some ongoing questions, however:

• Should we change the name of the various coefficient parameters for kernels to be functor instead? I haven't made that change yet, as it could be argued that "coefficient" refers to the mathematical concept rather than the MFEM class in this case.
• Is it actually appropriate to expose the MFEMXXXCoefficientName types to the end-user? We are pretending coefficients are functors for them, so maybe we should just use MooseFunctorName, or similar.
• Some parts of the MOOSE docs (e.g., documentation for the functor parameter in FunctorDirichletBC) say that functors can just be numbers. However, this is not mentioned in the main functor documentation page. I'm not clear entirely what this means anyway. That named constants can be used? How would we support this?

@alexanderianblair @Heinrich-BR Do either of you have any comments?

[cmacmackin]

I've made some further changes to simplify things, ensuring that all creation of coefficients is done through the CoefficientManager class. These, in turn, create all coefficients through the CoefficientMap class. The CoefficientMap objects are private members of CoefficientManager, so no one else will be able to use them. All of this means there will be just one way to do things, hopefully making it clearer for future developers and reducing the maintenance burden.

[cmacmackin]

This is almost ready for review. I'd like to add one or two integration tests that use an (aux)variable as a coefficient for a kernel. The only things left to do are answer the queries above.

[alexanderianblair]

I've handled the comments I raised on particular parts of the code. Some ongoing questions, however:

* Should we change the name of the various `coefficient` parameters for kernels to be `functor` instead? I haven't made that change yet, as it could be argued that "coefficient" refers to the mathematical concept rather than the MFEM class in this case.

* Is it actually appropriate to expose the `MFEMXXXCoefficientName` types to the end-user? We are pretending coefficients are functors for them, so maybe we should just use `MooseFunctorName`, or similar.

* _Some_ parts of the MOOSE docs (e.g., documentation for the `functor` parameter in [FunctorDirichletBC](https://mooseframework.inl.gov/source/bcs/FunctorDirichletBC.html)) say that functors can just be numbers. However, this is not mentioned in the [main functor documentation page](https://mooseframework.inl.gov/syntax/Functors/index.html). I'm not clear entirely what this means anyway. That named constants can be used? How would we support this?

@alexanderianblair @Heinrich-BR Do either of you have any comments?

1. I personally prefer usage of 'coefficient' rather than 'functor' everywhere since it's referring to the mathematical concept from a user perspective - 'coefficient' is clearer than 'functor' for a new user IMO. But I'd definitely keep this consistent across BCs and kernels.

2. I'd keep things as MFEMXXXCoefficientName personally - they're still not quite MOOSE functors, so I quite like the extra clarity that it's taking in the name of an XXXCoefficient. I would try to be consistent

3. I suspect this is the usage of functors where a value can be passed in and the relevant (constant) object is built under the hood if a value is passed in rather than a name (eg. here) - for us that'd be creating an mfem::ConstantCoefficient. We could do this in the same way to Functors by modifying the getScalarCoefficient method to check if the name is parsed to a constant (see here)
   This would reduce duplication of kernels/BCs by combining the 'regular' BCs/Kernels with the functor ones.

[cmacmackin]

I personally prefer usage of 'coefficient' rather than 'functor' everywhere since it's referring to the mathematical concept from a user perspective - 'coefficient' is clearer than 'functor' for a new user IMO. But I'd definitely keep this consistent across BCs and kernels.

Which is more important: maintaining consistency with existing functor BCs in MOOSE (in which case we'd say "functor") or using the clearer term (in which case we'd say "coefficient")?

I suspect this is the usage of functors where a value can be passed in and the relevant (constant) object is built under the hood if a value is passed in rather than a name (eg. here) - for us that'd be creating an mfem::ConstantCoefficient. We could do this in the same way to Functors by modifying the getScalarCoefficient method to check if the name is parsed to a constant (see here)
This would reduce duplication of kernels/BCs by combining the 'regular' BCs/Kernels with the functor ones.

I like this idea, but let's leave it for a follow-up pull request.

[cmacmackin]

@alexanderianblair Technically you're down as the author of this PR, so I can't send you a request to perform a review. Could you leave any comments and once you're satisfied with everything I'll give an approving review to tick the boxes on GitHub.

[alexanderianblair]

Which is more important: maintaining consistency with existing functor BCs in MOOSE (in which case we'd say "functor") or using the clearer term (in which case we'd say "coefficient")?

Apologies, I wasn't clear here - I meant the latter. MOOSE isn't consistent on this front either (FVDiffusion uses coeff instead of functor), and coefficient would be consistent with the underlying type being used by the BCs and kernels.

I like this idea, but let's leave it for a follow-up pull request.

Yup - sounds good to me, to keep this PR from getting any longer!

[cmacmackin]

I've changed the names of those parameters from "functor" to "coefficient". It looks like the version of VTK being used in the build image differs from that used to create the comparison output files now, though, and this is causing the tests to fail. @alexanderianblair, @Heinrich-BR, any idea what's going on there?

 #17 11.26 test:kernels.MFEMGravity: ################################################################################
#17 11.26 test:kernels.MFEMGravity: Begin tester output
#17 11.26 test:kernels.MFEMGravity: 
#17 11.26 test:kernels.MFEMGravity: Schema difference detected.
#17 11.26 test:kernels.MFEMGravity: File 1: /opt/platypus/test/tests/kernels/gold/OutputData/Gravity/Run0/Run0.pvd
#17 11.26 test:kernels.MFEMGravity: File 2: /opt/platypus/test/tests/kernels/OutputData/Gravity/Run0/Run0.pvd
#17 11.26 test:kernels.MFEMGravity: Errors:
#17 11.26 test:kernels.MFEMGravity: Value of root['VTKFile']['@version'] changed from "0.1" to "2.2".
#17 11.26 test:kernels.MFEMGravity: Tester failed, reason: SCHEMADIFF
#17 11.26 test:kernels.MFEMGravity: 
#17 11.26 test:kernels.MFEMGravity: ################################################################################
#17 11.26 test:kernels.MFEMGravity: End tester output
#17 11.26 test:kernels.MFEMGravity: 
#17 11.26 test:kernels.MFEMGravity ................................................................. FAILED (SCHEMADIFF)

[alexanderianblair]

I've changed the names of those parameters from "functor" to "coefficient". It looks like the version of VTK being used in the build image differs from that used to create the comparison output files now, though, and this is causing the tests to fail. @alexanderianblair, @Heinrich-BR, any idea what's going on there?

 #17 11.26 test:kernels.MFEMGravity: ################################################################################
#17 11.26 test:kernels.MFEMGravity: Begin tester output
#17 11.26 test:kernels.MFEMGravity: 
#17 11.26 test:kernels.MFEMGravity: Schema difference detected.
#17 11.26 test:kernels.MFEMGravity: File 1: /opt/platypus/test/tests/kernels/gold/OutputData/Gravity/Run0/Run0.pvd
#17 11.26 test:kernels.MFEMGravity: File 2: /opt/platypus/test/tests/kernels/OutputData/Gravity/Run0/Run0.pvd
#17 11.26 test:kernels.MFEMGravity: Errors:
#17 11.26 test:kernels.MFEMGravity: Value of root['VTKFile']['@version'] changed from "0.1" to "2.2".
#17 11.26 test:kernels.MFEMGravity: Tester failed, reason: SCHEMADIFF
#17 11.26 test:kernels.MFEMGravity: 
#17 11.26 test:kernels.MFEMGravity: ################################################################################
#17 11.26 test:kernels.MFEMGravity: End tester output
#17 11.26 test:kernels.MFEMGravity: 
#17 11.26 test:kernels.MFEMGravity ................................................................. FAILED (SCHEMADIFF)

Looks like it's a side-effect from the upstream merge of mfem/mfem#4797 into MFEM's master branch - probably easiest to pin the MFEM commit in the platypus-deps Dockerfile to just before this than to re-gold all .pvd files - probably best in a separate PR

[alexanderianblair]

Main functionality looks good - I'd update the reference to 'functors' in documentation to refer to coefficients where appropriate, and see if MOOSE's unique names could be used instead of the pointer to string conversion, but otherwise happy. Major QoL improvement for access and set-up of coefficients!

[cmacmackin]

I've made the requested changes. I'll let the CI run and you do a final check.

[alexanderianblair]

LGTM! Thanks