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

[Core] Adding a method to fetch the non-converged solutions from the NewtonRaphson strategy #12375

Merged
merged 24 commits into from
May 23, 2024

Conversation

Rbravo555
Copy link
Member

@Rbravo555 Rbravo555 commented May 13, 2024

📝 Description

This PR adds the possibility to store all nonconvereged solutions during the iterative procedure. After either convergence has been achieved or the maximum number of iterations has been reached, one can fetch a Kratos Matrix containing the nonconvereged solutions. The relevant method also returns the dof set in case the matrix should be treated.

If the Setup method is not called, no overhead is introduced.

🆕 Changelog

  • Added a setup method to trigger the storage of the nonconverged solutions
  • Added a get method that returns both the matrix of nonconvered solutions and the dofs set
  • Added a test inside the test_strategies.cpp

Sub-optimal as it requires variables to be passed. A better solution should simply gather variables from the dofset
Copy link
Member

@loumalouomega loumalouomega left a comment

Choose a reason for hiding this comment

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

Some comments

Rbravo555 and others added 8 commits May 14, 2024 12:26
…son_strategy.h

Co-authored-by: Vicente Mataix Ferrándiz <vmataix@altair.com>
…son_strategy.h

Co-authored-by: Vicente Mataix Ferrándiz <vmataix@altair.com>
…son_strategy.h

Co-authored-by: Vicente Mataix Ferrándiz <vmataix@altair.com>
…son_strategy.h

Co-authored-by: Vicente Mataix Ferrándiz <vmataix@altair.com>
@Rbravo555 Rbravo555 marked this pull request as ready for review May 14, 2024 11:00
@Rbravo555 Rbravo555 requested a review from a team as a code owner May 14, 2024 11:00
@rubenzorrilla
Copy link
Member

Shouldn't we add this to the base implicit strategy?

Comment on lines 909 to 910
void SetUpNonconvergedSolutionsGathering(){
mStoreNonconvergedSolutionsFlag = true;
Copy link
Member

Choose a reason for hiding this comment

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

It is a good practice to make the member var match the name of the inquiry and set methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Method's name now matches the flag's

* @brief Sets up the gathering of non-converged solutions
* @details This method enables the storage of non-converged solutions at each iteration by setting the appropriate flag.
*/
void SetUpNonconvergedSolutionsGathering(){
Copy link
Member

Choose a reason for hiding this comment

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

You should pass the bool to be set in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Passing a boolean now

* @return A vector containing the current solution values for the given DOF set
* @details This method retrieves the current solution values for the provided DOF set. Each value is accessed using the equation ID associated with each DOF.
*/
Vector GetCurrentSolution(DofsArrayType& rDofSet){
Copy link
Member

Choose a reason for hiding this comment

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

I think we should pass a reference in here. Otherwise we are creating a copy at return time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reference is now passed to avoid copies

*/
Vector GetCurrentSolution(DofsArrayType& rDofSet){
Vector this_solution(rDofSet.size());
for (auto& r_dof : rDofSet) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (auto& r_dof : rDofSet) {
for (auto& r_dof : rDofSet) {

Can't this be done in parallel?

Copy link
Member Author

Choose a reason for hiding this comment

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

using block for each now

@Rbravo555
Copy link
Member Author

Shouldn't we add this to the base implicit strategy?

The base class does not implement iterations. By adding this here, we ensure that non-converged solutions are collected for both the Newton-Raphson and Line Search strategies. Many applications do not re-implement this method in derived strategies, so we thought implementing it here is the most suitable approach.

@matekelemen
Copy link
Contributor

Eeeh honestly this seems to me like a hack. I like the idea, but I'd rather add infrastructure to allow hooking output processes into the strategy base class. Is something like that possible?

@RiccardoRossi
Copy link
Member

Eeeh honestly this seems to me like a hack. I like the idea, but I'd rather add infrastructure to allow hooking output processes into the strategy base class. Is something like that possible?

Well this would be a major change. As we discussed we should definitely do a redesign of the strategies etc and we could think about your proposal as a part of that, nevertheless this is a small, completely backward compatible, change. My proposal would be to accept it "as is" and discuss your proposal in the context of a "strategy v2", for example during your stay in BCN in 10 days from now

@matekelemen
Copy link
Contributor

Fine by me.

@RiccardoRossi
Copy link
Member

then i am approving

@Rbravo555 Rbravo555 merged commit 6709900 into master May 23, 2024
11 checks passed
@Rbravo555 Rbravo555 deleted the core/nonconvereged_solutions_for_nr_strategy branch May 23, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants