Addresses issue when running Samurai-TR #120
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## thermo_asap #120 +/- ##
===============================================
+ Coverage 32.21% 32.22% +0.01%
===============================================
Files 53 53
Lines 18406 18396 -10
===============================================
Hits 5929 5929
+ Misses 12477 12467 -10 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @johnmauff for fixing the numVars issue.
Besides one minor question, I am curious about the output of your new Beltrami run with these changes. Does the residual / iteration count remain the same?
| @@ -47,7 +47,7 @@ set(SOLVER_CG_EPSILON 1.0e-18) | |||
| set(SOLVER_CG_BETA_TYPE 2) | |||
|
|
|||
| # type of convergenve for Samurai CG(1 = ~step size(orig), 2 = ||g(X)||/||g(X0)||) | |||
| set(SOLVER_CG_CONV_TYPE 1) | |||
| set(SOLVER_CG_CONV_TYPE 2) | |||
There was a problem hiding this comment.
Is there a reason to change the convergence type here?
There was a problem hiding this comment.
The previous setting of convergence type is the original value before the code was modified by ASAP. I don't know how it got set to 1 but the more numerical proper value is setting to be 2. Allison kept the old option in there but it is really not numerically correct. I suspect that changing this will modify iteration count, though I have not tested it yet.
There was a problem hiding this comment.
Thanks for the clarification. Since the SOLVER_SAMURAI is currently set to TN solver, I assume this change won't affect the results at all unless we change the solver type?
There was a problem hiding this comment.
Good point, I missed that this only applies to the CG solver. Now I understand why it was set to 1 originally.
There was a problem hiding this comment.
From the scientific side, Michael and I had also identified numVars as a potential source of error, so 7 should be correct.
The new updated sounding file is correct.
I'm less knowledgeable about the SOLVER_CG_CONV_TYPE variable requirement, so I don't feel comfortable weighing in on that change.
This PR addresses an issue in the construction of the observation vector that was creating non-checkboard solutions. It also updates the output of Samurai-WIND and uses the correct convergence criterion.