Remove references to coefficients from boundary conditions

[cmacmackin]

This finally removes the MFEMCoefficient classes. That involves refactoring the boundary condition classes. There are now two versions of each type of boundary condition: one which takes constant values and one which takes a function name. It is a little messy needing to have these duplicate classes, but it is in-line with how MOOSE does these things.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
src/base/PlatypusApp.C	86.36% <ø> (-2.32%)	⬇️
src/bcs/MFEMConvectiveHeatFluxBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMScalarDirichletBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMScalarFunctionDirichletBC.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/MFEMVectorFunctionBoundaryIntegratedBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMVectorFunctionDirichletBC.C	100.00% <100.00%> (ø)
src/bcs/MFEMVectorFunctionDirichletBCBase.C	100.00% <100.00%> (ø)
... and 9 more

[cmacmackin]

@alexanderianblair I'd like to add some more unit tests, to cover the essential BCs. However, it's not clear to me how we should do that. Reading the GridFunction documentation, it doesn't look like it's trivial to check that a Dirichlet BC has been applied to the correct boundary. Do you have any suggestions?

[alexanderianblair]

Essential BCs remove (true) DoFs from the bilinear form (operator) corresponding to that variable in the EquationSystem - to check that the Dirichlet condition has been applied to the correct boundary, you could get the array of true DoFs from the GridFunction's ParFESpace (via fespace.GetEssentialTrueDofs(ess_bdr, ess_tdof_list) and check these DoFs on the trial GridFunction are set to the expected values from the BC after the FormLinearSystem step.

For a more robust test, you could also check that the essential DoFs have indeed been eliminated from the bilinear form of a test problem - may be overkill given existing tests in MFEM, though.

[cmacmackin]

@alexanderianblair I've written some initial tests for the Dirichlet boundary conditions (currently just checking that everything runs, not checking it actually does what's expected). The tests for tangential vector Dirichlet boundaries keep segfaulting and I'm at a bit of a loss as to why. I suspect it's because something hasn't been initialised correctly in the tests. Could you take a look and let me know if anything immediately stand out to you?

[alexanderianblair]

Looks like it's the FESpace being used - the tangential vector BC is expecting a FE type with a vector basis (ND), so is failing because it's trying to set scalar DoFs in the FESpace using a 3D vector at each DoF.

Using an FE variable based on an H1_FECollection for the nodal Dirichlet condition and a vectorial FE variable based on an ND_FECollection should fix this

[cmacmackin]

I think I've fixed the segfaults (on my computer at least). I'm not quite clear how to add the checks you described above, Would you be able to provide an example? That would probably be faster than trying to figure it out myself.

[lindsayad]

I personally like what you guys had before better. Just because MOOSE has different classes for constant, function, material property, postprocessor, etc. based coefficients doesn't mean you should change what you're doing. We've been actually moving towards more class consolidation in MOOSE by using the functor system, from which variables, functions, functor material properties, and postprocessors all derive so that we don't need quadruplicate classes to cover what a user might leverage in their input file

[lindsayad]

So there are currently three coefficient accessors on the problem (I think) that MFEM objects can use:

• getProperties().getScalarProperty()
• getXXXFunctionCoefficient()
• makeXXXCoefficient()

In practice all these objects care about is whether the coefficient is a Coefficient, VectorCoefficient, or MatrixCoefficient, so it would be neat if there was only one set of APIs for getting coefficients: getScalarCoefficient, getVectorCoefficient, getMatrixCoefficient. And then correspondingly you should have a smaller set of MFEM objects (like you did before) that do not care about whether the (Vector/Matrix)Coefficient was created through a function, material property, etc.

[lindsayad]

I personally enjoyed being able to use the same Coefficient object in multiple input file blocks in #62

[alexanderianblair]

We've been actually moving towards more class consolidation in MOOSE by using the functor system, from which variables, functions, functor material properties, and postprocessors all derive so that we don't need quadruplicate classes to cover what a user might leverage in their input file

This is useful to know that this is the direction MOOSE is going in - we definitely could have FunctionCoefficients added to the PropertiesManager, allowing a consistent point of access for usage downstream (via getProperties().getXXXProperty()), which would keep the flexibility that you mention above for using coefficients in multiple blocks In that case, there are a few options for the design here:

1. We add FunctionCoefficients and VectorFunctionCoefficients to the associated PropertiesManager, so coefficients generated by materials, functions, or otherwise are all stored in the same place. This keeps MOOSE-like syntax for the setup of those coefficients, but a user wouldn't be able to name a function and a material the same name without issues, assuming the coefficient name is taken to be the same. Not necessarily the most intuitive for MOOSE users that a function or a material should be passed into downstream block as the coefficient input, but avoids the issues of duplication of downstream classes depending on their supported input.

2. We handle everything as Coefficients directly in the input - in which case we need to handle the dependencies between coefficients carefully when coefficients depend on each other (eg. for PWCoefficients used to represent materials). Setting up material properties in the Coefficients block like this would diverge from the familiar MOOSE method of setting up block restricted parameters in materials, which would be a barrier to entry. However, the name of the coefficients in the system is unambiguous this way.

IMO Option 1 would be more familiar for existing MOOSE users, and sounds closer to the UI MOOSE would have after moving to functors.

[cmacmackin]

I don't think it is correct to view properties and coefficients as the same things. Properties are are represented using coefficients, but they are subtly different. Properties are inherently piecewise, being defined on particular blocks. Even in the case where they are defined over all blocks, they are expressing information about what blocks they apply to. Coefficients, on the other hand, have no information about the blocks they apply to.

The point of the PropertyManager class is to handle the assembling of properties gradually, as the input file is parsed. We don't find out all at once what the value/expression for each property is on each block; that gets defined one material at a time. There was previously a rather messy way to assemble that information (which I can't even remember, to be honest). The PropertyManager class keeps it simple for the end-user.

I do not think it would be appropriate to mix the creation of Coefficients in with the PropertyManager class as it exists now. I suppose it would be possible to refactor it into some sort of coefficient manager class which takes on additional responsibilities, but I'm concerned that would lead to confusion about the distinction between coefficients and properties that I have described.

[cmacmackin]

I personally like what you guys had before better. Just because MOOSE has different classes for constant, function, material property, postprocessor, etc. based coefficients doesn't mean you should change what you're doing. We've been actually moving towards more class consolidation in MOOSE by using the functor system, from which variables, functions, functor material properties, and postprocessors all derive so that we don't need quadruplicate classes to cover what a user might leverage in their input file

I think it would be useful to have a clearer understanding of how that new approach will look in MOOSE then. I think ensuring consistency is an important virtue, so we should aim to make our approach match whatever will be coming in MOOSE.

[lindsayad]

Coefficients, on the other hand, have no information about the blocks they apply to.

This isn't true, is it? Don't you guys leverage PWCoefficient::UpdateCoefficient for particular mesh attributes/blocks? I guess I do not see any difference between block-restrictable material properties and PWCoefficient

[alexanderianblair]

After discussion with @cmacmackin: we're conscious of the fact that Coefficients don't exist as a concept in vanilla MOOSE. As a result, we're keen to avoid them appearing as separate objects in HIT inputs as things currently stand, to minimise the barrier to entry for MOOSE users trying out the use of MFEM as an FE backend. That said, reducing the duplication of downstream objects (like BCs and Kernels) is very appealing.

Current plan of attack is:

• To remove Coefficients from HIT input files explicitly, by merging this PR as-is (following rebase)
• Open a new PR to investigate use of a unified Coefficients store in MFEMProblem, to avoid duplication of the input interface of BCs and Kernels for Functions and Materials. Possibly using MFEMFunctorXXXBC or MFEMFunctorXXXKernel classes, to keep the analogy with MOOSE.

[lindsayad]

The plan sounds reasonable to me

• Open a new PR to investigate use of a unified Coefficients store in MFEMProblem, to avoid duplication of the input interface of BCs and Kernels for Functions and Materials. Possibly using MFEMFunctorXXXBC or MFEMFunctorXXXKernel classes, to keep the analogy with MOOSE.

Yea I like this. To elaborate more, I'd be fine with multiple database being unique/shared owners of different kinds of Coefficients. This is the case for MOOSE functors ... the variable warehouse owns variables, the function warehouse owns functions, etc. But then we have one container which doesn't have any ownership but holds pointers to all the different functors in the MOOSE simulation. This is the container that is queried using a single interface function (getFunctor) by MooseObjects. I imagine something similar could be done here for Coefficients

[cmacmackin]

@alexanderianblair Could you run Clang-format on the code to get the linting working?

[alexanderianblair]

This looks like a good intermediate step, removing Coefficients from user input files ahead of the follow-up PR introducing Platypus's equivalent to FunctorBCs.
Tests now all pass as expected.