-
Notifications
You must be signed in to change notification settings - Fork 249
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
Adding features for release of eFlows4HPC European Project #10081
Conversation
@@ -2,21 +2,36 @@ | |||
import KratosMultiphysics.FluidDynamicsApplication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes go against the latest modifications. Implementations must be done in the rom_analysis
. Note that this class is deprecated and is only keep for retrocompatibility.
Is there any technical limitation that avoids implementing these new features in the rom_analysis
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rom_analysis
that we have now works well for testing small cases, since it allows to train a ROM quite automatically. However, this automatic workflow was not inmmediatly usable in a larger parallel training campain.
That is, the rom_analysis
builds the ROMs using a single time-dependent, or a single quasi-static simulation(with multiple load-steps). If more parameter variations are to be studied, one needs to compute the SVD of the snapshots for the whole set of simulations. The scripts for such a larger parallel training are not included in this PR, since they involve the external library COMPSs. They are being place in the Kratos/Examples repository.
Incorporating an expanded workflow for the training of a ROM in the rom_analysis
will be done in another PR.
Notice that the modifications on the fluid_dynamic_analysis_rom
add but a single extra functionality in comparison to the rom_analysis
. The PATH is accepted as an extra parameter, in order to allow to run in a cluster. All the real handling is done externally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorporating an expanded workflow for the training of a ROM in the
rom_analysis
will be done in another PR.
If there is no technical limitation, can't we just simply do this?. From the sake of code sustainability it makes no sense to invest effort on developing and maintaining deprecated code.
Notice that the modifications on the
fluid_dynamic_analysis_rom
add but a single extra functionality in comparison to therom_analysis
. The PATH is accepted as an extra parameter, in order to allow to run in a cluster. All the real handling is done externally.
This can be easily added to current rom_analysis
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a brief discussion with @Rbravo555 about this. As the due date of the eFlows4HPC release is close, we decided to do the release with the old stage version to ensure everything is working fine. This comes with the compromise all these developments to the current rom_analysis
after the deliverable submission.
applications/RomApplication/tests/test_randomized_singular_value_decomposition_mpi.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 If no other comments I will approve, since we need a release soon
📝 Description
Adds features for parallel computation of SVD and Simulations
🆕 Changelog
fluid_dynamic_analysis_rom
to ease its launching in clusters (no extra tests added, since it depends on external library COMPSs)