Allow MooseObjectAction to extract parameter from its corresponding parent folder

[YaqiWang]

Reason

This small (one-line change) yet possibly controversial feature can be done really easily in Parser as

diff --git a/framework/src/parser/Parser.C b/framework/src/parser/Parser.C
index b267ce9711..b9010312a7 100644
--- a/framework/src/parser/Parser.C
+++ b/framework/src/parser/Parser.C
@@ -367,6 +367,7 @@ Parser::walkRaw(std::string /*fullpath*/, std::string /*nodepath*/, hit::Node *
       {
         object_action->getObjectParams().blockLocation() = params.blockLocation();
         object_action->getObjectParams().blockFullpath() = params.blockFullpath();
+        extractParams(MooseUtils::baseName(curr_identifier), object_action->getObjectParams());
         extractParams(curr_identifier, object_action->getObjectParams());
         object_action->getObjectParams()
             .set<std::vector<std::string>>("control_tags")

This basically allows users to put common parameters shared by for example materials under [Materials] block instead of repeatedly under their own sub-blocks. The feature is better than [GlobalParams] because those params within [Materials] can only been seen by materials. This limited scope can reduce the possibility of naming conflict among different types of objects.

Lots of neutronics models can benefit from this feature because of a number of materials could exist with some common parameters.

Design

This is a completely new feature in the input file and there could be some implications. But conceptually I think this feature is good.

Impact

A new feature.

[permcody]

Definitely controversial - this has come up several times. This will require a MOOSE team discussion. There are others that strongly dislike GlobalParams as well and I'm all for moving away from that feature.

[friedmud]

I thought this would happen automatically with HIT?

On Tue, Jul 23, 2019 at 5:03 PM Cody Permann notifications@github.com wrote:

Definitely controversial - this has come up several times. This will require a MOOSE team discussion. There are others that strongly dislike GlobalParams as well and I'm all for moving away from that feature.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/13783?email_source=notifications&email_token=AAGUSMLNKSHDQGRXBG4PZRTQA6E3LA5CNFSM4IGJJCJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2UVR2Y#issuecomment-514414827, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGUSMIESIJCNQZCAW5HQY3QA6E3LANCNFSM4IGJJCJQ .

-- Sent from my iPhone

[permcody]

HIT can do this. It’s just a question of introducing this syntax into MOOSE. I advocated for this a long time ago but I seem to remember some discussion about whether we should or not.

On Tue, Jul 23, 2019 at 6:01 PM Derek Gaston notifications@github.com wrote:

I thought this would happen automatically with HIT?

On Tue, Jul 23, 2019 at 5:03 PM Cody Permann notifications@github.com wrote:

Definitely controversial - this has come up several times. This will require a MOOSE team discussion. There are others that strongly dislike GlobalParams as well and I'm all for moving away from that feature.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/idaholab/moose/issues/13783?email_source=notifications&email_token=AAGUSMLNKSHDQGRXBG4PZRTQA6E3LA5CNFSM4IGJJCJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2UVR2Y#issuecomment-514414827 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAGUSMIESIJCNQZCAW5HQY3QA6E3LANCNFSM4IGJJCJQ

.

-- Sent from my iPhone

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/13783?email_source=notifications&email_token=AAXFOIEB6NI6CSJVPUHTUKLQA6LVTA5CNFSM4IGJJCJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2UYOLY#issuecomment-514426671, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXFOIAYZILU5ZQYVL4KNSDQA6LVTANCNFSM4IGJJCJQ .

[dschwen]

I'd like this. I have hacked that capability into the tensor mechanics master action but having it on a global scale would be convenient and consistent.

[permcody]

So it looks like we may do this. However, this patch is not what we want. ~Matching only the last part of the name will mean that we can pick up pieces of input file syntax from completely unrelated blocks~. Matching only the base name isn't quite right. You want this to work up the whole hierarchy if we have multiply nested blocks. If we do implement this, we'll do it correctly.

[YaqiWang]

I think one level up is possibly enough. It is safer. If there are special needs, application side can do something. I was thinking replacing AddMaterialAction for this initially then thought this could be a useful feature in MOOSE.

[permcody]

I disagree - you only have to look around for a few minutes to see cases where we have a few levels of nesting and you might want a parameter at the base level to apply all the way down. Think about the BCs block for instance that has the extra level for Periodic. Imagine how bizarre it would be if you had a parameter at the base that applied to all your BCs except the Periodic ones just because of the level. We either do it all or none - it's got to be consistent.

[YaqiWang]

Sounds a good argument. So putting parameters ONLY on level 1 (level 0 is kind of what GlobalParam is doing) sounds a good idea.

[friedmud]

I still want this to work for GlobalParams. GlobalParams would simply just become variables at the outermost scope.

On Wed, Jul 24, 2019 at 10:21 AM Yaqi notifications@github.com wrote:

Sounds a good argument. So putting parameters ONLY on level 1 (level 0 is kind of what GlobalParam is doing) sounds a good idea.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/13783?email_source=notifications&email_token=AAGUSMP2VQ4IADWHFQOXD7DQBB6PBA5CNFSM4IGJJCJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2W3TVQ#issuecomment-514701782, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGUSMNFBRMLWBOKGRRXLT3QBB6PBANCNFSM4IGJJCJQ .

-- Sent from my iPhone

[YaqiWang]

Can we make this available to users? GlobalParams can be really confusing. It is good for code developers, but for normal users, they have no global picture of all the valid parameters of all objects. On top of this, GlobalParams can sometimes create unexpected behavior if two objects shares the same parameter name but doing completely different things with it. I do not want remove GlobalParams, but want to give users another option to simplify their inputs and make the inputs more readable. Thanks.

[lindsayad]

Do we need to have a design meeting about this? It sounds like what was conceptually agreed upon here could be achieved with @YaqiWang's one-liner plus a MooseUtils::basestName (I don't think I like rootName) which does something like find_first_of('/') instead of find_last_of('/').

[YaqiWang]

Maybe just start a conversation on slack? @lindsayad when this is ready, can you search GlobalParams in Griffin repo and replace them with this? We should not have alot input files in Griffin for this.

[dschwen]

does that mean

dim = 3
[Mesh]
  type = GeneratedMesh
[]

would be valid?

[dschwen]

Also, the TensorMechanicsAction already does something like that by registering an action to the parent syntax path and querying the action warehouse to obtain the parameters. We'll have to be careful there are no clashes with that approach.

[dschwen]

Thirdly, a better design approach would be to traverse all levels upwards, not just one parent level. Deeper level parameters should always have priority over higher level parameters.

At that point it would make sense to completely remove [GlobalParams].

The problem is that we would not have a separate namespace for input file variables that currently reside outside top-level blocks (unless we add kludges specific naming conventions: start with underscore etc)!

[YaqiWang]

Possibly better to change TensorMechanicsAction in this PR to use this new capability. Regarding level 0, we can wrap the whole thing with

[main_app]
[]

If we have globally defined names used with $(), we will have to add this level, otherwise the code can internally do this. GlobalParams block can be deprecated. This actually make multiapp easier to setup because all inputs can share one single input file.

[lindsayad]

Thirdly, a better design approach would be to traverse all levels upwards, not just one parent level. Deeper level parameters should always have priority over higher level parameters.

@dschwen take a look at at #15759 . In https://github.com/idaholab/moose/pull/15759/commits/7234ffd559322bfc6ff9deef8c6d60a807148f7a, I went ahead and traversed all levels, with lower levels overriding higher levels.

That, as implemented, would not handle params provided outside top-level blocks.

[dschwen]

How do we handle cases where bot parent and child blocks have valid params (such as Executioner/TimeStepper). It seems odd to permit Timstepper parameters in the Executioner block. Even worse if both have overlapping parameter names and you want to set a value in parent but do not want it to be set by user in child. That would be impossible now.

[lindsayad]

How do we handle cases where bot parent and child blocks have valid params (such as Executioner/TimeStepper). It seems odd to permit Timstepper parameters in the Executioner block.

It does? If so, then that begs the question to me of why we have it nested in that location in the first place.

Even worse if both have overlapping parameter names and you want to set a value in parent but do not want it to be set by user in child.

You don't want it to be set by the user, period? I would use suppressParameter for this. But I'm guessing you're thinking of some other scenario. My small brain is having a hard time concocting a scenario where you would want to forbid a user setting a parameter via a higher-level block, but not forbid setting it at the lowest level.

[dschwen]

I do not want to forbid the user from doing anything! The scenario is that a nested class performs an isParamSetByUser check on a parameter that is also a valid parameter of the parent object.

[lindsayad]

The scenario is that a nested class performs an isParamSetByUser check on a parameter that is also a valid parameter of the parent object.

And you only want this to return true if the value was set in the nested class's block? I think that's legitimate, but I don't know how to protect that user while also granting the capability requested by this ticket. I could add the 10 millionth API to InputParameters : isParamSetByAnyPartOfTheInputFile and then isParamSetByUser would only correspond to setting in the nested class. Or I could add isParamSetByBottomMostBlock and then isParamSetByUser would mean was the parameter set anywhere in the input file.

[YaqiWang]

We might change syntax.registerActionSyntax a little that when we associate an input syntax, do we want the syntax to use this capability. Executioner/TimeStepper for example can be a syntax should not use upper level parameters.

[rwcarlsen]

I don't like this. I posted this on the PR too, but I'll paste it here for discussion flow:

The opaque pushing of parameters down into unknown sets of objects config makes input files harder to understand and debug. If I see foo=bar inside a materials block that has 8 materials, how do I know which materials are using it? I prefer a pull approach. Objects should use substitution expressions where they can pull in parameters from parent scopes. This already works and has the cascading parent scope traversal with inner scope overriding. It also supports referencing params outside the enclosing scopes via foo/bar/baz syntax which the PR/proposal doesn't support.

In short, I believe the existing code (zero new lines) already provides a superior alternative to everything proposed here (hundreds of new lines of code).

[rwcarlsen]

To be clear - the pull-approach is less convenient for the input file writer. But it is far more convenient for the input file reader/interpreter. Since input files are read more than written, I feel like it is a worthy tradeoff. Make the file slightly more onerous to write but the end result is easier to read and understand.

[lindsayad]

So @rwcarlsen, something like this works right now?

[Materials]
  prop_val = 5
  [gen_1]
    type = GenericConstantMaterial
    prop_names = 'prop1'
    prop_values = '${Materials/prop_val}'
  []
  [gen_2]
    type = GenericConstantMaterial
    prop_names = 'prop2'
    prop_values = '${Materials/prop_val}'
  []
[]

Let me know if my concept is right but the syntax is wrong...

[rwcarlsen]

That will work, but you can also do this:

[foo]
  bar = baz
[]
[Materials]
  prop_val = 5
  bar = 42
  [gen_1]
    type = GenericConstantMaterial
    prop_names = 'prop1'
    prop_values = '${prop_val}'
  []
  [gen_2]
    type = GenericConstantMaterial
    prop_names = 'prop2'
    prop_values = '${prop_val}'
  []
  [gen_3]
    type = GenericConstantMaterial
    prop_names = 'prop2'
    prop_values = '${foo/bar}'
  []
  [gen_4]
    type = GenericConstantMaterial
    prop_names = 'prop2'
    prop_values = '${bar}'
  []
[]

I think there might be some unfortunate warnings/errors (depending on cli flags) though for having out-of-line parameters in your input file not at the global level. But that is something that should be relatively straight forward to address.

[lindsayad]

So in the above example the prop value in gen_4 would have the value of 42? E.g. the correct one wins?

This all sounds fine to me, but it obviously doesn't shorten the length of the input file, which was what @YaqiWang was after. @YaqiWang are you persuaded by the argument of the ease of reading input files vs. the ease of writing them?

[rwcarlsen]

Yes - gen4 gets 42, gen3 gets baz. If gen4 had its own bar param directly in it, that would override the outer "42" one as well.

[YaqiWang]

I want prop_values = '${prop_val}' to be removed. Parser will automatically grab the available parameter in the upper levels. Of course parameter on the closest level wins. We can allow this prop_values = '${foo/bar}', since it is overriding the default rule.

[YaqiWang]

With this in place, we possibly can deprecate GlobalParams.

[YaqiWang]

An unrelated question, if we have a parameter contains 100 zeros and 100 ones, can we do something like parameter = '@repeat{100, 0} @repeat{100, 1}'?

[rwcarlsen]

We could also possibly add a new brace expression like foo=${auto} that evaluates to foo=${foo}? @YaqiWang - you didn't say anything about the concerns and tradeoffs between push vs pull mechanisms for parameter sharing. You just reiterated what you want. I've frequently had a lot of trouble trying to follow/understand input files that other people wrote (or that I wrote not recently) that make heavy use of this. I see various things in global params and have no clue what objects those parameters are affecting. The proposal here is slightly less bad in that respect - in that it allows people to (if they choose to) put the global params a bit closer scope-wise to the affected objects, but the same fundamental issue remains. If you have lots of objects that are extremely inconvenient to use this way, then a better solution would probably be to wrap things up with some sort of action syntax. I really think explicit pull/substitution expressions are much more clear and provide larger payoffs over time that more than compensate for the initial inconvenience.

There is a mechanism to add custom brace expressions to the parser - it is very straight forward to add things like this. But the syntax would be ${repeat 100 0} ${repeat 100 1}. You could definitely open an issue to discuss options.

[YaqiWang]

Explicit pull is nice in sense of making the input clearer, but it defeats the original purpose of shortening the inputs. We could have for example of 20 materials that share a few exactly same parameters. We want put these parameters under Materials. Adding a custom action inside applications to have this capability is not a good idea because this seems a common interest of several independent developers. This capability can be badly abused like the case of GlobalParams, which makes users not know which leaf input block is using which parameters provided by GlobalParams. This is the reason I suggested before to only allow one level of pull and only restricted to MooseObjectAction. One level pull is fine to me. I have indicated this before: I dislike GlobalParams.

[lindsayad]

We may need a general weigh-in from the @idaholab/moose-ccb on this one. Introduce push or no? :+1: or :-1: ? I see value in the arguments on both sides.

[YaqiWang]

We can disable more-than-one-level automatic pull and allow explicit pull at the same time. I do not mind to require explicit pulls for parameters inside GlobalParams.

[friedmud]

I agree with Yaqi - explicit pull defeats the purpose.

But restricting it to one level is too restrictive too. There are lots of places in MOOSE where there are more than two levels of possible hierarchy.

GlobalParams should work just as if the values are at the top level. We can deprecate GlobalParams later... but right now it's in tons of input files.

On Thu, Aug 20, 2020 at 5:17 PM Yaqi notifications@github.com wrote:

We can disable more-than-one-level automatic pull and allow explicit pull at the same time. I do not mind to require explicit pulls for parameters inside GlobalParams.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/13783#issuecomment-677952408, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGUSMNCSVQQANWV7GRJYFTSBWVIBANCNFSM4IGJJCJQ .

[rwcarlsen]

@friedmud - what are your thoughts about the tradeoffs/downsides of implicit push?

[rwcarlsen]

At least with globalparams - you know exactly where to look for the implicitly pushed params. If we just allow it everywhere - it will make what I consider an anti-feature of input files even worse.

[YaqiWang]

We can add debug print-outs to indicate implicit pull happened. Can be controlled by a parameter like Debug/show_implicit_parameter_pull. If a user is unfamiliar with an input, he can turn this on to see what exactly happened and have better idea on the input file. With this I guess random placing the implicitly pushed params won't be a big concern as well.

[friedmud]

We do already have --show-input to see how MOOSE interpreted the input file... that would be a quick way for people to check.

On Wed, Aug 26, 2020 at 4:17 PM Yaqi notifications@github.com wrote:

We can add debug print-outs to indicate implicit pull happened. Can be controlled by a parameter like Debug/show_implicit_parameter_pull. If a user is unfamiliar with an input, he can turn this on to see what exactly happened and have better idea on the input file. With this I guess random placing the implicitly pushed params won't be a big concern as well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/idaholab/moose/issues/13783#issuecomment-681152229, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGUSMJU2EXA5DATF7M64ZDSCWCV3ANCNFSM4IGJJCJQ .

[YaqiWang]

Oh, never tried that, but if we can show only the implicit pulls, it will be great because the input file could be pretty lengthy making things not so obvious.

[rwcarlsen]

@YaqiWang - That just seems sort of like finding a solution to a problem we gave to ourselves - when the better way would be to just not give ourselves the problem in the first place. I want to be able to understand/interpret an input file easily using just the input file. I would really prefer to limit the circumstances where using various external input analysis binaries/tools are required in order to figure out what is going on.

[YaqiWang]

I do have mixed feelings about this. The use case for me or Griffin is that we used to have lots of block-restricted materials sharing bunch of same parameters. But this case can be solved by doing material assignment with the newly added capability of element integers. The desire level of this capability for me is not as strong as of before. And I do see the difficulty that this creates for documentation, easily understandable input, etc. I should have mentioned that I do not want to have a new action in Griffin to replace AddMaterialAction in MOOSE either because that could create racing conditions if two apps are to be linked together and both asking replacement of an action behind the same input syntax. I will leave the decision of this PR to your guys. And hopefully the decision is final. I tend to vote removing GlobalParams completely, which can possibly create incentives for app developers to rethink the user interface more carefully.

You would think removing GlobalParams is difficult, but it may not be. The parser can do the substitution through pre-processing the inputs. This can be part of the input formatter. Thanks.

[rwcarlsen]

A while ago, global params were really causing me trouble in some input files, so I wrote a moose parser patch that automatically rewrote the input files to move the global params into every object that used them. If we really wanted to deprecate/remove global params, then maybe something like that could help us accomplish it.

[lindsayad]

I say we deprecate them. We could also get some other users opinions

On Sep 1, 2020, at 9:18 AM, Robert Carlsen notifications@github.com wrote:

 A while ago, global params were really causing me trouble in some input files, so I wrote a moose parser patch that automatically rewrote the input files to move the global params into every object that used them. If we really wanted to deprecate/remove global params, then maybe something like that could help us accomplish it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[joshuahansel]

I've gotten into trouble with GlobalParams before as well. However, I also really like the ability to share parameters. I think that just having parameters be a little less global would go a long way, i.e., delete GlobalParams and instead require it to be at the block level, e.g., Materials, or lower. However, if others feel strongly against this, the explicit substitution is ok - we can live with that.

[friedmud]

Some real calculations heavily utilize globalparams: such as some solid mechanics calcs. They are handy at times.

I still vote for keeping the capability to have globally defined values... regardless of how that's implemented.

Like any input file syntax or programming language: you can always use it poorly. But that's up to you! Hell, in C++ we could make all variables global if we wanted to (like in the old Fortran common block days)... but we just don't do that most of the time because we've all learned that it's not the best way to program. However, there are still times that truly global variables are useful - and, indeed, we even have some in MOOSE (factories, etc. for instance).

[GiudGiud]

How about a compromise: automatic pull but with an error if the parameter is not used. This should cover the huge problem with generalized implicit pull @rwcarlsen pointed out.

That way:

• input files can be smaller, just group smartly blocks that use the same parameters
• less risk of forgetting an attribute in a block since they are shared
• somewhat reduced use of GlobalParams by everyone since this new way would be more convenient and generally better syntax

AND no headaches at debug time. If a parameter is above in a block hierarchy, then it is used. The error message, when the parameter would apply to a block that does not want it, should be something like "Parameter xxx implicitly pulled by block_with_doesnt_usexxx is not used. Move this parameter definition so that it doesn't apply to block..._...".

Note that we'll have to do something smart for nested blocks, if the parameter is used in the inner block, then it shouldn't error out on the outer block. Note also that this should not apply to variables defined at the beginning of the input file, which should still be explicitly pulled.

Example: OK (illustrates the overriding possibility, and the possibility to define variables mid-block)

[Variables]
  order = FIRST
  family = MONOMIAL
  [power_distribution]
    family = LAGRANGE
    fv = true
  []
  order = CONSTANT # from now on all blocks will see CONSTANT instead of FIRST
  fv = true. # could have been earlier, but this is ok too!
  block = '3 4' # applies only to the two blocks below
  [temp_fluid]
    initial_condition = 800.0
  []
  [vel_x]
    initial_condition = 0.0
  []
[]

Not OK (inlet_vel_y Receiver does not use a function attribute)

[Postprocessors]
   function = '0'
  [inlet_vel_y]
    type = Receiver
    default = ${inlet_vel_y}
    execute_on = INITIAL
  []
  [L1_norm]
    type = ElementL1Error
    variable = power_distribution
  []
[]

[rwcarlsen]

@GiudGiud - there are good and bad points to writing input files that way with this proposal.

say I have:

[Mats]
  shared_param = 42
  [foo]
     type = ...
  []
  [bar]
    type = ...
  []
[]

What if I were unaware that bar also had a param named shared_param - I would be surprised that it was also having it's default behavior overridden. Or what if, I want to revert to using bar's default behavior - but now instead of commenting out or deleting a single shared_param line in bar, I now have to add an extra line to foo and comment out or delete shared_param from the higher level materials block. Also, at a glance, readers still have the problem that they don't know which objects may be using a higher-level-block-provided parameter (edit - I see your proposal actually somewhat resolves this issue, but how often are there parameters that apply to every sub-block?). Also (much less significant points) there are issues of implementation complexity etc.

My primary concern remains input file reading/comprehension. For anyone who hasn't recently written the input file or is not intimately familiar with all the objects being used, explicit parameter pulling will make understanding input files much more straight forward and quick.

[GiudGiud]

You would not be surprised because if 'bar' did not also have a param named shared_param, the code would error out (based on my proposal). I see the concern though. We could absolutely address it by adding a warning when a shared param overrides a default (and no warning for required params).

You also would not need to comment out the shared param to revert to the default behavior, only add a single line to bar, since this overrides the shared param.

The point of my proposal is to address these two issues you mention. I agree we could use a reset mechanism to end a shared param. Maybe shared_param_name = NONE ?

I agree having a shared_param before a hierarchy of sub-blocks and knowing where it applies would be difficult. How deep is the typical block structure though? So far I haven't seen more than one level of nesting.

"All shared params have to apply to all blocks in the same scope below them, unless overridden" is a summary of the proposal. I think that's simple enough that people will be able to match parameters to their blocks when reading an input file.

Also, the input file length is a concern for input file comprehension/debugging. Repeated parameters in the input files are a major contributor to their lengths.