ginkgo-project / ginkgo

Numerical linear algebra software package
https://ginkgo-project.github.io/
BSD 3-Clause "New" or "Revised" License
409 stars 88 forks source link

Add Convergence history #1517

Closed MarcelKoch closed 3 months ago

MarcelKoch commented 11 months ago

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.
sonarcloud[bot] commented 11 months ago

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

upsj commented 9 months ago

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

MarcelKoch commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 3 months ago

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