idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.77k stars 1.05k forks source link

Get rid of `variable` in (nonlinear) kernels, and name this parameter `equation` #24524

Open GiudGiud opened 1 year ago

GiudGiud commented 1 year ago

Reason

It's a confusion that has lived on since the beginning. We have to explain it over and over again to users that are adding coupled (think coupledKernels) terms in multiphysics problems. It's a barrier to understanding MOOSE in depth, a gotcha that does not need to be

EDIT: variable for AuxKernel is totally fine

Design

Massive parameter rename, deprecation of 100% of all moose inputs We could use the name of the variable to name the equation at first (easy default for compatibility) Besides, equations are naturally tied to Systems. So this make it easier to do do multi-systems from the input file

Impact

Improve user understanding of MOOSE by a few per cent. Lessen confusion for multiphysics users

lindsayad commented 1 year ago

@idaholab/moose-ccb

cticenhour commented 1 year ago

Considering that kernels reflect the outcome of an operator performing its operation on a particular variable, with potentially several variables being referenced in a particular equation (albeit one being the "primary" variable in a given PDE), I'm not sure I like the suggested swap in the title here.

You want an equation to be the object-level reference to its primary variable, with coupled variable parameters being expressed the same as they are now? I think this adds confusion and complexity.

GiudGiud commented 1 year ago

AuxKernels perform an operation that sets a particular variable, so they totally fit your definition. From non linear kernels to the value of a nonlinear variable, it is not so straightforward. These kernels fit better a definition of "something central that performs a single operation" but not on a variable, here on residual & jacobian

The "primary" variable is also quite arbitrary. It works out for most equations, notably thanks to the time derivative term. One could easily think of an equation where it would not be clear (X + Y = f(t) for example). The impact however is very significant, because it determines which terms are on the diagonal and which are not.

You want an equation to be the object-level reference to its primary variable

I dont think we need to do that. We just need to have an object/concept for equations, and an object for variables. Doing what you mention just seems like the mirror of our current confusion (referring to the variable when talking about the equation)

It certainly adds a tad of complexity to reference both a variable and an equation (think for example a basic diffusion kernel, where you'd need two parameters now). But it could help to get rid of a lot of the "coupledXX" objects if the equation was different from the variable.

Thinking more about the coupledXX objects. Imagine you have a X div(grad(Y)) term. If you want it in the equation for Y, you use coupledDiffusion. If you want it in the equation for X, you use coupledMatReaction with a div(grad(Y)) material property most likely. This kind of gymnastics is not necessary if the equation and the variables are different

joshuahansel commented 1 year ago

In those cases where you're having to duplicate a class to use something other than the equation variable in a term, you can just add another parameter variable_to_use_in_some_term and achieve the same effect as what you suggest, right? Then you should be able to delete the redundant class? Then it's just a matter of whether the parameter names are intuitive.

I agree that "variable" may not always be intuitive (maybe "equation_variable", "residual_variable", or "associated_variable" might be more descriptive names), but I think this change is a bit extreme compared to the potential clarification.

Perhaps you can give some specific examples?

GiudGiud commented 1 year ago

just add another parameter variable_to_use_in_some_term and achieve the same effect as what you suggest

but then effectively, you are using a variable parameter which really means equation. Think of that one https://mooseframework.inl.gov/source/kernels/ADMatCoupledForce.html (or most coupledKernel really). We set a variable parameter, but the variable doesnt even show up in the term. It's v that shows up.

I agree this does not bring additional capability. Just clarifies the concepts of equations and variables.

joshuahansel commented 1 year ago

but then effectively, you are using a variable parameter which really means equation.

Yeah, I agree this would be better named as something more descriptive, but I think "variable" is not terrible at least...

For CoupledForce, I recommend renaming v to force_variable, or maybe just force (a Functor value) 😈 There's also the possibility of using the parameter renaming thing to change "variable" to "equation_variable" in one class if not clear, right? I don't know that it would be a good practice to do this (probably not), but it's a consideration.

lindsayad commented 1 year ago

I think that having variable somewhere in the name does have value as you will be supplying a parameter value corresponding to a variable. Something like equation_variable like @joshuahansel suggested

GiudGiud commented 1 year ago

It doesn't have to be a variable's name if we introduce the concept/system of Equation. I could see:

[Variables/T]
[]
[AuxVariables/q]
[]
[Equations]
  [heat_conduction]
    primary_variable = T
    coupled_variables = q # (maybe unnecessary but could do some parameter checking?)
  []
[]
[Kernels (or whatever the new name ^^)]
  [diff]
    type = Diffusion
    equation = heat_conduction
    variable = T
  []
  [source]
    type = HeatSource
    equation = heat_conduction
    value = q
  []
[]

very nicely, we stop referring to T in the heat source so there's no way to get it wrong. Because the HeatSource kernel is not actually operating on variable T, in the context of a coupled multiphysics problem it s operating on more than that one variable (here it s just T, and even here, it acts on residuals/jacobian not T)

This also accommodates better some NS models where all the kernels are written in terms of material properties, so variable parameters are just pointing to the equation there too

lindsayad commented 1 year ago

In order for users to build effective multiphysics calculations, they often need to think about the disparate scaling between physics residuals and jacobians. Whatever we do should not decrease the amount they think about this. I'm not saying what you're proposing does that.

In the example of a residual that is like X * Y, sure the decision about what variable to add the residual and Jacobian to is highly arbitrary, but the decision has real significant meaning depending on the residual/Jacobian scale compared to whatever terms exist for the chosen variable/equation