[Feedback]: Subdomain-Restricted Variables Implementation Plan

[pbauman]

We have an urgent need to support this capability (for FSI, but it will be neat to also easily be able to do conjugate heat transfer). I've been thinking about how to do this and I wanted to outline my plan here for feedback (parts that are a bad idea, things I missed, etc.).

First, this will not be possible with the "old style" of variable input (in the Physics section), but in this plan, that style will still be preserved, but it won't get the new capability.

I was thinking the input should look like the following:

[Variables]
   [./Displacement]
      names = 'Ux Uy'
      order = 'FIRST'
      fe_family = 'LAGRANGE'
      enabled_subdomains = '1 2'
   [../]
[]

The enabled_subdomains would be optional - if it's not present, then it is assumed that the Variable is enabled for all subdomains.

For construction of Variables, I was thinking that instead of having the Physics construct them (as it is now), I can make a VariableBuilder that uses a VariableFactory that handles the construction of Variable objects and registers them with the VariableWarehouse. At Variable construction time, before the Physics enters the picture, we can already have the names, order, type, and subdomains parsed and added to the System. A nice side-effect of this is that it will be much easier for a user to add their own Variable.

Then, the Physics just queries the VariableWarehouse. This will allow error checking for things like making sure the Variable is enabled for all the subdomains for which the Physics is enabled. Then, the Physics can still internally set the is_constraint_var(), neumann_bc_sign, and (to be implemented) is_time_varying() (so we can remove the set_time_evolving_vars() out of the Physics) using setter functions.

The VariableBuilder would be similar style to what I did in #403 where there's the "OldStyle" (which would just be extracting the bits from the current FEVariableBase hierarchy) and the DefaultVariableBuilder. The benefit of this, beyond getting Variable construction logic out of the Physics and not having duplicate Variable objects floating around, is we make the FEVariableBase objects more lightweight: the Builder and Factories worry about construction so we only put what the Physics and boundary conditions need to access in the FEVariableBase.

In particular, I think FEVariableBase should only need to provide access to: variable indices, variable names, subdomain ids, material (actually there would be a map between the material and subdomain ids); I need the material in the catalytic wall boundary conditions to pick out the thermochemistry library type.

Additional error checking that will need to be added is to make sure that if a boundary condition is enabled for a subdomain-restricted variable, then we need to make sure that the boundary is actually a boundary of elements that contain elements of that subdomain. This check can easily be added in the BCFactory base classes (which, thus, requires building this on top of #403).

Additionally, because of the need for the material name in the catalytic wall BCs, we need to make sure that the given boundary id only touches elements with the same material. This is a check we can do in the boundary condition construction that would mandate this (the catalytic walls) by querying the boundary_info, buildling the element list for that boundary id, and make sure that the subdomain ids in that element list correspond to that material in the Variable class.

I'm thinking I can do this in two stages: 1. Implement the Builder/Factory pattern with the current capabilities and then 2. Add the subdomain restricted parts.

I'm going to go ahead and start working on 1., but I'll take comments into account as they roll in. Thanks in advance for any feedback.

[pbauman]

One important point that I forgot to include.

I think we need to also have the option (requirement?) that the user sets the Variable name in the input, e.g.

[Physics]
...
    [./HeatConduction]
       material = 'Coal'
       variables = 'Temperature'
    [../]
...
[]

I'm envisioning situations where, for example, we have different sets of species in different parts of the domain, but the Physics is still ReactingLowMachNavierStokes so we need to allow the usage of different variables within the same Physics.

Certainly in the old style of parsing we'll maintain a default. But the new version, we can choose whether to require it or not. I'm leaning towards requiring it because that will allow the stripping away of all the FEVariablesBase subclasses and we have minimal code there and can deploy helper functions in the Physics if we want the syntatic sugar like the _disp_vars.u() we have now.

All of this is open for discussion of course.

[pbauman]

Hmm, actually, we still strip all the subclasses away, put in the syntatic sugar in the Physics and still have a default for the Physics/<PhysicsName>/variables value.

[pbauman]

One hiccup with arbitrary variable naming will be the axisymmetric boundary conditions. With a physical variable name, we can automate what to do for the boundary condition. If we take that away, we'll need to rethink that choice.

[roystgnr]

Could we specify a "variable type" which is a physical variable name (e.g. "Temperature" and has a default name (e.g. "T"), but also allow an optional arbitrary variable name to override the latter?

[pbauman]

Damn good suggestion, have a variable_type makes a lot sense.

[pbauman]

Unlike my writing today...

[pbauman]

OK, this is going well, but there are a couple of points for consideration/discussion. Below, "before" means the current state of master, and "after" means my work-in-progress. Also, FEVariablesBase is the name of the objects, but I also generically refer to Variables below to mean, essentially the same thing.

• Before, the order of variable insertion to the System was dependent on 1. The order of the Physics in the enabled_physics argument (user-changeable) and 2. The order of the variable.init() calls in the Physics (not user-changeable). After, it will depend only on the order of the subsections of [Variables]. e.g.

  [Variables]
  [./Velocity]
  ...
  [../]
  [./Temperature]
  ...
  [../]
  []
  

  In the above, Velocity variables will be added to the System first, and then the Temperature. I like this because now the user has complete control of the ordering of the Variables in the System, e.g. to group all the like variables together for efficiency. However, this can lead to some surprises. 1. Your solver behavior may change from what you had before the Variable ordering changed (e.g. if the position of the pressure block changed, this will effect naive preconditioners) 2. If you have a restart and you don't respect the variable ordering of the restart, you're gonna have a bad time.

• We haven't done this previously, but maybe what we can do now that we're factoring the Variable construction out of the Physics, is if the user has a restart file, we parse the restart, and then the System is setup with all the Variable info, so we populate the FEVariablesBase according to the restart and then build up the Physics (since "after" the Physics will just grab the FEVariablesBase objects from the previously built-up VariableWarehouse). Throw a warning message if the ordering of the variables in the system and the input file is different? Throw an error if variables are missing from either input or System?
• For "OldStyle" input, i.e. where the FE type, order, etc. were set in the Physics section, I can preserve the Variable ordering in the "OldStyle" parser. If you're moving to the new style parser, then you just need to be aware of the caveats above. For the "in-between" state where the Variables section is present, like @nicholasmalaya is in, I'm not quite sure what to do. In particular, AveragedTurbineBase hasn't moved to using a FEVariablesBase object yet, so it's not possible to preserve the variable ordering in the in-between state without forcing the addition of the Variable subsection in the input file for that turbine variable. If @nicholasmalaya can live with having to add that variable to the input file (without it, currently, there'd be an error), then this will work. If not, then I'm not sure how to proceed here, caveat dealing with restarts as discussed above. I mean, I guess I could try parsing the Physics as well and look for AveragedTurbineBase subclasses and force the construction of the turbine variable in the right place, but it would be nice to not have to do that. :)
• Actually, since @nicholasmalaya has already moved away from specifying Variable info the Physics section, maybe we just dump the "OldStyle" completely?

[pbauman]

We haven't done this previously, but maybe what we can do now that we're factoring the Variable construction out of the Physics, is if the user has a restart file, we parse the restart, and then the System is setup with all the Variable info, so we populate the FEVariablesBase according to the restart and then build up the Physics (since "after" the Physics will just grab the FEVariablesBase objects from the previously built-up VariableWarehouse).

Oh, wait, crap. The System has to have the variables added before we can read the restart?

[roystgnr]

The system certainly isn't supposed to have to have variables added before you read a restart, unless you didn't turn on the headers when writing the restart.

[pbauman]

The system certainly isn't supposed to have to have variables added before you read a restart, unless you didn't turn on the headers when writing the restart.

Ah, OK, great. I think that pulling in Variable info from the restart, if it's there, should be very doable then. I think that doing so would be a big improvement and remove a lot of potential for errors.

[pbauman]

What about tagging a release after #403 is merged? That would give a reference point before the Variable change that would break input compatibility with AverageTurbineBase subclasses (and ScalarODE as it turns out).

[pbauman]

OK, part 2 (the actual subdomain-restricted variable part) is in #409. It includes a conjugate heat transfer example + regression test to exercise the capability. Input is as suggested here.

[pbauman]

All this work is done and merged.