pmgbergen / porepy

Python Simulation Tool for Fractured and Deformable Porous Media
GNU General Public License v3.0
251 stars 88 forks source link

Use separate parameter dictionaries for model and numerics setup #1210

Closed keileg closed 1 month ago

keileg commented 3 months ago

Right now, standard practice is not to split parameters into physical and numerical (solver-related) ones, we typically do:

params = {'model_param_1': 'foo', 'solver_param': 'newton'}
model = FooModel(params)
pp.run_time_dependent_model(model, params)

This soon leads to a cluttered parameter dictionary and is doomed to introduce inconveniences in naming etc. down the road. A better approach would be to split the two,

model_params = {'model_param_1': 'foo'}
solver_params = {'solver_param': 'newton'}
model = FooModel(model_params)
pp.run_time_dependent_model(model, solver_params)

It is not clear if this is possible at the moment, or if we have made assumptions that means we cannot do such a splitting. This should be investigated. A splitting will require that we define a boundary between the two, which may not always be possible; again we need to understand which problems this may trigger before moving in this direction.

jwboth commented 3 months ago

Related to #1150

jwboth commented 3 months ago

Task: Answer the question: “Is it possible to make a clean split of the parameter dictionaries into model and solver parameters?” The answer most likely is “yes”, having a look at the parameters used in the solver. If so, update the tutorials and tests using split parameter dictionaries. Recommended: Use your daily simulations for a proper investigation.

Main outcome: Knowledge about ability to split the parameter dictionaries into two disjointed sets. Infrastructure for model initialization and solution strategy to use different types of parameter dictionaries. Updated tutorials and tests.

isakhammer commented 3 months ago

The prepare_simulation boolean parameter holds a high level of importance within the parameter hierarchy, as it not only initializes solver parameters but also configures materials, which are highly model-dependent. A possible improvement could be to decouple the set_materialsfunction from the solution strategy, allowing for a clearer separation.

image

mikeljordan commented 3 months ago

Quick glance at some of the params in both solver and physical model, this splitting works by testing with compositional model settings. However, we should also consider to have time_manager as part of solver_params. This should not be too hard to decouple from the physical model params.

Screenshot 2024-08-15 at 14 13 28

mikeljordan commented 3 months ago

Branch containing fixes to this issue is fix-issue-#1210.

jwboth commented 3 months ago

Thank you, Micheal. I suggest, you continue for now. And once, you feel you either get close to finish or willing to share a WIP status of the code, just make a PR. This will make it easier to comment directly to the code changes.

isakhammer commented 3 months ago

image

Any strong opinions on renamingparams to solver_params? While it may require some minor renaming throughout the codebase, I believe it aligns better with the goal of this issue.

keileg commented 2 months ago

Any strong opinions on renamingparams to solver_params? While it may require some minor renaming throughout the codebase, I believe it aligns better with the goal of this issue.

Makes sense to me. @IvarStefansson @jwboth what do you think?

IvarStefansson commented 2 months ago

I agree, as long as it is only used for solvers.

jwboth commented 2 months ago

I also support the suggestion. The way I understood, this issue will be tackled, the solver_params will truely contain only solver parameters. So, regarding Ivar's fear can be silenced.