[SofaBaseLinearSolver] FIX breaking condition in CG at first step regarding threshold #556
Conversation
If threshold is met at first step with den=0, break but info msg about the status of the linear system. If threshold is met at first with den!=0, then continue one more iteration and warning about threshold value too large.
hugtalbot
added
pr: fix
|
[ci-build][with-scene-tests] |
There was a problem hiding this comment.
Looks OK to me. Basically, apart from issuing a warning, the code should not explicitly handle the case when the residual is below a threshold already in the first iteration, since numerically, it's a correct behavior even if it yields an unwanted result (e.g., an object is not moving despite applied forces). CG is an iterative solver and anyone using it should be well aware of its parameters and their impact on the simulation.
|
[ci-build][with-scene-tests] |
hugtalbot
changed the title
|
Your remarks are taken into account @IPeterlik :) |
| endcond = "threshold"; | ||
| if( verbose ) | ||
| { | ||
| sout<<"CGLinearSolver, den = "<<den<<", smallDenominatorThreshold = "<<f_smallDenominatorThreshold.getValue()<<sendl; | ||
| msg_info() << "CGLinearSolver, den = " << den <<", smallDenominatorThreshold = " << f_smallDenominatorThreshold.getValue(); |
There was a problem hiding this comment.
No need for CGLinearSolver...msg_info add the prefix.
| sout<<"den = "<<den<<", alpha = "<<alpha<<sendl; | ||
| sout<<"x : "<<x<<sendl; | ||
| sout<<"r : "<<r<<sendl; | ||
| msg_info() << "den = " << den << ", alpha = " << alpha; |
There was a problem hiding this comment.
No needs for three messages...one is better (and more efficient)
| @@ -63,6 +63,29 @@ CGLinearSolver<TMatrix,TVector>::CGLinearSolver() | |||
| f_smallDenominatorThreshold.setRequired(true); | |||
| } | |||
|
|
|||
| template<class TMatrix, class TVector> | |||
| void CGLinearSolver<TMatrix,TVector>::init() | |||
There was a problem hiding this comment.
For usability reason I would recommand to have something like
if(f_verbose.getValue()){
f_printLog.setValue(true);
}| @@ -109,7 +132,7 @@ void CGLinearSolver<TMatrix,TVector>::solve(Matrix& M, Vector& x, Vector& b) | |||
| double rho, rho_1=0, alpha, beta; | |||
|
|
|||
|
|
|||
| msg_info_when(verbose) <<" CGLinearSolver, b = "<< b ; | |||
| msg_info_when(verbose) << " CGLinearSolver, b = " << b ; | |||
There was a problem hiding this comment.
No needs for the 'CGLinearSolver' prefix
damienmarchal
added
the
pr: fast merge
|
Now it is fast merge :) |
|
is this fine now ? |
|
For me, when the CI will re-vive and says ok it may go. |
|
[ci-build] |
|
[ci-build][with-scene-tests] |
|
oh dear lord this PR looks fine ! |
|
[ci-build][with-scene-tests] |
|
Agreed, let's merge! |
|
sorry again for the inconvenience of this sexy # in my PR branch name.. |
guparan
changed the title
hugtalbot
added
pr: status ready
Fix bug introduced in #521
If threshold is met at first step with den=0, break but info msg
about the status of the linear system.
If threshold is met at first with den!=0, then continue one more iteration
and warning about threshold value too large.
This PR:
Reviewers will merge only if all these checks are true.