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

Add Convergence history #1517

Closed
wants to merge 2 commits into from
Closed

Add Convergence history #1517

wants to merge 2 commits into from

Conversation

MarcelKoch
Copy link
Member

This PR enables the Convergence logger to also keep a history of the residual vectors, residual norms, and implicit squared residual norms. There are three modes,

  1. no history is kept,
  2. only norms are kept,
  3. norms and vectors are kept.

@MarcelKoch MarcelKoch self-assigned this Dec 20, 2023
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. labels Dec 20, 2023
@MarcelKoch MarcelKoch requested a review from a team December 20, 2023 15:05
Copy link

sonarcloud bot commented Dec 22, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@fritzgoebel fritzgoebel left a comment

Choose a reason for hiding this comment

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

LGTM!
I am not the biggest fan of having an std::vector<std::unique_ptr<LinOp>> for the residual nor history since it is a bit unintuitive to read from. I would prefer having for example an array<ValueType> of the norms, for multiple right hand sides this could be strided and have e.g. the last norm as padding for the earlier converged cases.

@upsj
Copy link
Member

upsj commented Feb 12, 2024

I'm wondering, instead of modes, should we have distinct classes for the three cases? Their use cases seem pretty distinct

@MarcelKoch
Copy link
Member Author

@upsj IMO, all cases still fit the Convergence logging theme, so it could be argued that they are not too distinct. I would feel that different classes will make it harder to find the functionality. If there is only one class with different constructor parameter, that seems to be a bit easier to me.

@upsj
Copy link
Member

upsj commented Feb 13, 2024

The three modes have three different use cases to me, as well as different interface requirements:

  1. final residual norm only: Numerical control, adaptive algorithms etc. in practical applications. Interface (simplified) get_norm()
  2. residual norm history (another question is whether we need the full history or just a truncated one): Convergence analysis, more of a theoretical tool, not sure how useful this is for production runs, maybe for more advanced adaptive control. Interface get_norms()[i] or get_norm_vector/matrix()
  3. residuals and norms (same question about truncation): Seems to me mainly suited for debugging, interface get_norms() and get_residuals()

That and the question of the representation that Fritz raised (should the output be stored in a compact format as a single matrix, should we allocate a fixed size and truncate the output to the latest residuals only?) seem important to discuss.

@MarcelKoch
Copy link
Member Author

I'm a bit hesitant to use different classes, because they are subtyping each other. In your example, 3. implements 2. and 1., 2. implements 1., so this would lead to the question of class hierarchy. I don't want to do that here, since I think it would complicate things too much, so I chose the single parameter to switch the behavior.

Regarding the compact or segmented storage, I don't see what benefit a full matrix would bring. I would also prefer to not store it as an array<ValueType> since that couples the value type too much with the class. Right now, it would be easy to remove the template parameter completely, and I think we should aim for that in 2.0. But I think the truncation would be a nice addition, at least with the vectors it would be easy to implement.

As @upsj the additional modes are mainly targeted for debugging purposes, and that's also why they were requested.

@MarcelKoch
Copy link
Member Author

I will close this PR as I think the functionality from #1620 is equivalent to this.

@MarcelKoch MarcelKoch closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod:core This is related to the core module. reg:testing This is related to testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants