Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let us control the pressure action #29603

Open
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

GiudGiud
Copy link
Contributor

@GiudGiud GiudGiud commented Dec 23, 2024

@jessecarterMOOSE can you look at the caveat in the documentation file and confirm it works for you?

It is fairly unconventional. @joshuahansel can you please take a look too?

We have 3 other options at least:

  • make the "enable" parameter private in the BCs object (when we create them from the action) so it can only be set from controlling the action. Therefore it no longer is surprising that they are all on / off at the same time
    EDIT: no current APIs to do that
  • create three enable parameters in the action and forward those to each BC. Seems not very useful
  • use the control_tag parameter only (no enable in the Action). The control tag is shared between all 3 BCs created, so <control_tag>/enable should be the way to refer to all 3 parameters together. control_tags are pretty clever but I'm fairly sure they are even less used than Controls

Whichever solution we find here might end up being deployed largely for Physics. So we can control a Physics all at once

@GiudGiud GiudGiud force-pushed the PR_control_pressure branch from 479d04e to dfdaab3 Compare December 23, 2024 00:17
@moosebuild
Copy link
Contributor

moosebuild commented Dec 23, 2024

Job Documentation, step Docs: sync website on 59e325e wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Dec 23, 2024

Job Coverage, step Generate coverage on 59e325e wanted to post the following:

Framework coverage

e193a3 #29603 59e325
Total Total +/- New
Rate 85.25% 85.25% -0.00% -
Hits 108024 108023 -1 0
Misses 18686 18687 +1 0

Diff coverage report

Full coverage report

Modules coverage

Solid mechanics

e193a3 #29603 59e325
Total Total +/- New
Rate 84.97% 84.98% +0.01% 100.00%
Hits 28392 28404 +12 19
Misses 5022 5022 - 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

Job GCC min debug on dfdaab3 : invalidated by @GiudGiud

unrelated timeout

@GiudGiud GiudGiud self-assigned this Dec 23, 2024
@GiudGiud GiudGiud marked this pull request as ready for review December 23, 2024 12:02
@jessecarterMOOSE
Copy link
Contributor

I think I'm fine with your implementation @GiudGiud . I don't see a reason (or if it even makes sense) why someone would turn off one of the BC's (disp_x, _y, or _z) created by a Pressure BC and not the others. So I would be in favor of preventing the user from doing that in the first place (your first bullet point)

About your documentation note, you say that "enable" has to be explicitly set in the input file but I do not see that in the test case.

@joshuahansel
Copy link
Contributor

joshuahansel commented Dec 23, 2024

Yeah, we have problem with controlling components - they need to "forward" the controllable parameters. We do this as with Component::connectObject:

  MooseObjectParameterName alias("component", component_name, component_param, "::");
  MooseObjectParameterName obj_param_full(obj_params.get<std::string>("_moose_base"), obj_name, obj_param);
  _app.getInputParameterWarehouse().addControllableParameterAlias(alias, obj_param_full);

However, I've noticed that trying to control Components/component_name/component_param in the input file doesn't work; instead you have to do component/component_name/component_param, so that part isn't great. If we could fix that, that would be the best solution to this problem.

@GiudGiud I will go ahead and see if I can figure out something here.

@joshuahansel
Copy link
Contributor

Ok, so I found that this was the solution to my problem at least:

    connectControllableParams("rho", obj_class_name, obj_name, "rho");

Does connectControllableParams not work in your situation? It looks like you wrote this method, so I'm guessing there's an issue.

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Dec 23, 2024

I don't recall writing this.
Does not seem to work here

test:pressure.3D: *** ERROR ***
test:pressure.3D: Unable to locate secondary parameter with name BoundaryCondition/Pressure_Side1_0/enable

seems the syntax isn't right. I think the true syntax is Pressure/BoundaryCondition/...
and it would not be BCs. It s because it uses the "moose_base" to form the syntax instead of the actual syntax

@GiudGiud
Copy link
Contributor Author

@jessecarterMOOSE ok I adapted the message. If there is no reason to control them separately then imo what we have here is fine.

@joshuahansel
Copy link
Contributor

What you have seems simple and effective. I might try moving components to use this approach too.

@joshuahansel
Copy link
Contributor

I just tried your same approach with components, but it has no effect. I don't understand how yours works. When you do

params.set<bool>("theparam) = getParam<bool>("theparam");

it's creating a copy of the initial value getParam<bool>("theparam"), not a reference, right?

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Jan 2, 2025

I'll double check.
Maybe we need a new routine for references

@GiudGiud GiudGiud force-pushed the PR_control_pressure branch from bb4a56b to a7db33a Compare January 10, 2025 19:23
@GiudGiud
Copy link
Contributor Author

" connectControllableParams("rho", obj_class_name, obj_name, "rho");"

is the correct solution here. Thanks @joshuahansel !
I'll deploy this to Physics at some point.

@GiudGiud GiudGiud force-pushed the PR_control_pressure branch from a7db33a to ccea8d9 Compare January 10, 2025 20:39
@jessecarterMOOSE
Copy link
Contributor

Could you generalize this? Seems useful for other actions that create kernels etc.

@GiudGiud
Copy link
Contributor Author

I think so for physics / actions that create equations.
Maybe the action warehouse should return parameters with already the connection between the two enable Boolean parameter (as soon as it exists for the Action, it always exists for the MooseObject)

@joshuahansel
Copy link
Contributor

I would vote against any sort of automatic parameter connection. I think in most cases you'd want a specific Physics parameter like enable_kernelA.

I think the only thing to improve would be to somehow connect when someone tries to control a parameter that doesn't actually do anything (like a Physics parameter that the developer forgot to connect to an object parameter), but I'm not sure how we'd detect that (without just using special logic for Physics etc. in the control system), so maybe not.

@GiudGiud
Copy link
Contributor Author

. I think in most cases you'd want a specific Physics parameter like enable_kernelA.

This could be great but I m not sure we can do if we have some sort Physics-wide "enable" (that ties the Physics enable to all the kernels, and probably just the kernels). I dont know that connectControllableParam can handle 2 separate connections for the same parameter

I think the only thing to improve would be to somehow connect when someone tries to control a parameter that doesn't actually do anything (like a Physics parameter that the developer forgot to connect to an object parameter), but I'm not sure how we'd detect that (without just using special logic for Physics etc. in the control system), so maybe not.

This would be great. I think it might be possible if we:

  • form a list of all parameters being controlled
  • check if any are part of the Physics
  • check if they are all "connected" to an actual object parameter

@joshuahansel
Copy link
Contributor

Ok, so I guess you mean Physics task that does the check, rather than some logic that lives in the control system. Yeah I think that's good. It doesn't need to be in this PR, but at least make an issue for it.

@GiudGiud
Copy link
Contributor Author

ok created #29687

@GiudGiud GiudGiud force-pushed the PR_control_pressure branch from ccea8d9 to 57f02e2 Compare January 13, 2025 22:46
@GiudGiud
Copy link
Contributor Author

ok took care of that last test failure I think. imo people need to stop inheriting classes but not their parameters...

Use the proper mechanics for forwarding
@GiudGiud GiudGiud force-pushed the PR_control_pressure branch from 57f02e2 to 59e325e Compare January 14, 2025 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants