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

[CoSim] Adding a strong block solver and block MVQN acceleration #11780

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

azzeddinetiba
Copy link

📝 Description
If I understand correctly, doing a block QN acceleration for CoSim coupling (using the PYTHON interface) is not possible as things are at the moment, since the acceleration is only applied after a full F(S(f)) ( Or S(F(u)) ) computation with the current coupled_solvers.
This is a suggestion to do that, with a block MVQN for example.
The idea is that the ConvergenceAcceleratorWrapper handles two CouplingInterfaceData instead of just one.

I realised ( probably too late 😅 ) that you aim to unify the accelerator interface with the ones in the Core or the ones in FSI (#5187) .. ? In which case this PR wouldn't be of help (?)

🆕 Changelog

  • Added block coupled solver
  • Added MVQN convergence acceleration in CoSim

@azzeddinetiba azzeddinetiba requested review from a team as code owners November 9, 2023 00:33
@azzeddinetiba
Copy link
Author

azzeddinetiba commented Nov 9, 2023

An example would be to change the config file on fsi_mok to

...

    "solver_settings" :
    {
        "type" : "coupled_solvers.block_strong",
        "echo_level" : 1,
        "num_coupling_iterations" : 12,
        "convergence_accelerators" : [
            {
                "type"      : "block_mvqn",
                "solver_sequence" :
                [
                    {
                        "solver": "fluid",
                        "data_name"  : "disp",
                        "coupled_data_name" : "load"
                    },
                    {
                        "solver": "structure",
                        "data_name"  : "load",
                        "coupled_data_name" : "disp"
                    }
                ]
                // "type"      : "mvqn",
                // "solver"    : "fluid",
                // "data_name" : "disp"
            }
        ],
        "convergence_criteria" : [
            {
                "type"          : "relative_norm_previous_residual",
                "solver"        : "structure",
                "data_name"     : "load",
                "abs_tolerance" : 1e-6,
                "rel_tolerance" : 1e-6
            }
        ],

...

        "coupling_sequence":
        [
            {
                "name": "structure",
                "input_data_list": [],
                "output_data_list": [
                    {
                        "data"           : "disp",
                        "to_solver"      : "fluid",
                        "to_solver_data" : "disp",
                        "data_transfer_operator" : "mapper"
                    }
                ]
            },
            {
                "name": "fluid",
                "input_data_list"  : [],
                "output_data_list" : [
                    {
                        "data"           : "load",
                        "to_solver"      : "structure",
                        "to_solver_data" : "load",
                        "data_transfer_operator" : "mapper",
                        "data_transfer_operator_options" : ["swap_sign"]
                    }
                ]
            }
        ],

...

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall I am fine with this addition, I dont think it would block the unification of the interfaces with the Core/FSIApp

But I have some conceptual questions

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay. Here are some more comments, style as well as conceptual:

  • Please use python-style for variable names whenever possible => snake_case
  • Please separate the wrapper for the block-accelerators, the code becomes quite messy otherwise
  • Generally I think this is a valuable addition, thanks!
  • Can you please add some tests?
    • For the wrapper I recommend doing similar tests to what I did for the current wrapper
    • For the accelerators ideally you would add a test like this one in the FSI-App. This tests the accelerators independently. I know that I never got to add it for the other accelerators, so this is optional. Maybe @rubenzorrilla can comment/recommend sth.
    • What about adding the mok-fsi case also for both of the new accelerators? This si a pretty small test case that prevent regressions, it runs an entire strongly coupled FSI simulation super fast

@azzeddinetiba azzeddinetiba marked this pull request as draft December 18, 2023 11:52
Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in terms of code it looks good now, thanks for the effort!

Will you add tests?

@azzeddinetiba
Copy link
Author

in terms of code it looks good now, thanks for the effort!

Will you add tests?

I still didn't take a look at the convergence wrapper tests.
For the fsi_mok test I think I can add one in a few moments. I'm thinking smth similar to the aitken one is sufficient ? without the external solver part ?

@azzeddinetiba
Copy link
Author

@philbucher is it possible to run a CoSim problem on MPI while the mapper is on serial compilation ?
Otherwise I can't test on my local machine, I can't compile Trilinos .. #11900

@azzeddinetiba azzeddinetiba marked this pull request as ready for review December 22, 2023 17:02
@azzeddinetiba azzeddinetiba marked this pull request as draft January 3, 2024 01:28
@azzeddinetiba azzeddinetiba marked this pull request as ready for review January 3, 2024 18:15
@philbucher
Copy link
Member

@philbucher is it possible to run a CoSim problem on MPI while the mapper is on serial compilation ? Otherwise I can't test on my local machine, I can't compile Trilinos .. #11900

Unfortunately not. I tried to implement this at some point, but decided it was not worth the effort, esp since we have Kratos-native matrices which can be used to completely replace trilinos

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thanks for the good work (and the patience)

@azzeddinetiba
Copy link
Author

Hi, can this be merged ?

@philbucher
Copy link
Member

sure
I approved months ago :)

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.

2 participants