Skip to content

Draft: feature/gridvars-based-assembly

Dennis Gläser requested to merge feature/elem-variables into master

What this MR does / why does DuMux need it:

This changes the assembly mechanism to operate on grid variables rather than solution vectors. This solves two issues:

  • the grid vars stored in the assembler, so far, were expected to be in sync with the solution vector passed into the assemble function. However, this was not really obvious.
  • the grid vars in the assembler were "magically" updated in the background in calls to newtonSolver.solve(x). This is now not magical anymore because one writes newtonSolver.solve(gridVars)

The old assembly behaviour looked something like this:

LocalAssembler la{assembler, element, curSol};
la.assemble(jacobian, gridVars...);  // so these grid vars should match with curSol, right?

I changed this to something like:

LocalAssembler la{assembler, element, gridVars};
la.assemble(jacobian);

The grid variables now expose a local view that internally contains local views on the grid volume variables and the flux vars cache. The new GridVariables contain information on the time level on which they are defined and this time level can be retrieved from the elemVariables, and thus, this provides the means to forward time information into problem interfaces as e.g. neumann.

GridVariables are now tied to one specific time level, the interface prevGridVolVars() has been removed. Instead, in this proposition one carries prevGridVariables in the main file (instead of xOld) and passes that to the assembler.

A general point of discussion:

  • In case we want this change, how should we proceed? Currently, this introduces breaking changes. I guess we have a few options:
      1. We could add this as separate classes/functions, (possibly) deprecating the old ones. Then, we could make more and more tests use the new interfaces bit by bit.
      1. We may make it nice, adapt all tests to the new style directly and merge -> this way we directly check how it plays out over all of dumux and test it thorougly. Independent on how it plays out in the context of the time bug, it may be viewed as an improvement for the time being. However, this would mean we make dumux master targetting 4.0..
      1. Basically do 2 but on a dedicated 4.0 branch that we leave open until we have fixed the time bug in follow-up-branches. However, we'll eventually have to rebase, keep things in sync, etc.
      1. We leave this on a branch in some intermediate state and keep working on the time bug in follow-up branches, and only finish all branches once we see that the steps actually do take us to fixing the bug for some selection of tests. The issue here is again that we'll have to do heavy rebasing etc (probably).

I personally think 2 would be the "easiest", although most cumbersome for users who want to keep their code working with master. But if we go for 2., I guess we'd have to make an announcement via the mailing list that we are entering a refactoring phase and that people should stay on a release branch for some time.

If we keep going with this, some more things to discuss:

  • neumann now has access to time info via the elemVars, but boundaryTypes and dirichlet don't. There, we should/could maybe directly pass in an instance of TimeLevel? We could make it auto boundaryTypes(const Element&, const Scv&, const std::optional<TimeLevel>& = {}) to make it explicit that this may be optional (i.e. in stationary settings it shouldn't be passed).
  • so far I only modified whatever was necessary for TPFA to work. Other schemes should also be modified.
  • in many places, this just forwards elemVars.elemVolVars() and elemVars.elemFluxVarsCache() to the old interfaces that expected them separately. Maybe there could be some harmonization done.
  • The element argument has been removed in some interfaces, because it is implicitly carried in fvElementGeometry. In the flux functions it was left in, because it could be some element of the stencil, not necessarily the bound one. On the other hand, via the scvf one could retrieve the inside element, if necessary. We could check if we could remove the argument from those interfaces as well and/or double-check if we shouldn't have removed them from some in the first place...
  • The assembler now receives a non-const reference to GridVariables to assemble the jacobian. That is not so nice, but was required because in the case of caching we deflect the grid vars directly. So far, the Assembler received a const ref to a solution, however, it carried a non-const reference to grid vars internally. So the current master has the same "issue", this change just makes it more explicit/obvious. Without a performance penalty (e.g. making a copy, which would introduce memory overhead) I don't see how to circumvent this, though.

Notes for the reviewer

TODO: insert text here

Before you request a review from someone, make sure to revise the following points:

  • does the new code follow the style guide?
  • do the test pipelines pass? (see guide on how to run pipelines for a merge request)
  • is the code you changed and/or the new code you wrote covered in the test suite? (if not, extend the existing tests or write new ones)
  • does your change affect public interfaces or behavior, or, does it introduce a new feature? If so, document the change in CHANGELOG.md.
  • is the list of the header includes complete? ("include what you use")
  • all files have to end with a \n character. Make sure there is no \ No newline at end of file comment in "Changes" of this MR.
  • (if not applicable remove) are newly introduced or modified physical values/functions backed up with a scientific reference (including doi) in the docs?
  • (if not applicable remove) if the examples are modified, is the documentation regenerated (using generate_example_docs.py)
Edited by Dennis Gläser

Merge request reports