<a href="http://petsc.cs.iit.edu/petsc/petsc-dev/rev/067c5dbc352e">http://petsc.cs.iit.edu/petsc/petsc-dev/rev/067c5dbc352e</a><br><br>Just a couple requests regarding this patch:<div><br></div><div>1. Please split independent functionality into separate patches. This does three different things.</div>
<div><br></div><div>2. Please cut out extra whitespace like this and follow conventions about not including args in the declarations.<br><br><div>@@ -273,6 +274,12 @@</div><div> PETSC_EXTERN PetscErrorCode KSPMonitorDefault(KSP,PetscInt,PetscReal,void *);</div>
<div> PETSC_EXTERN PetscErrorCode KSPLSQRMonitorDefault(KSP,PetscInt,PetscReal,void *);</div><div> PETSC_EXTERN PetscErrorCode KSPMonitorRange(KSP,PetscInt,PetscReal,void *);</div><div>+</div><div>+</div><div>+PETSC_EXTERN PetscErrorCode KSPMonitorDynamicTolerance(KSP ksp,PetscInt its,PetscReal fnorm,void *dummy);</div>
<div>+PETSC_EXTERN PetscErrorCode KSPMonitorDynamicToleranceDestroy(void **dummy);</div><div>+</div><div>+</div><div><br>3. This doesn't look like an improvement:<br><br><div>@@ -163,7 +163,7 @@</div><div>     /* scalar updates */</div>
<div>     omega = xi2 / xi3;</div><div>     beta = - xi4 / sigma;</div><div>-    rho = PetscSqrtReal(PetscAbsScalar(xi1 - omega * xi2)); /* residual norm */</div><div>+    rho = sqrt(fabs(xi1 - omega * xi2)); /* residual norm */</div>
<div> </div><div>     /* vector updates */</div></div></div><div><br></div><div><br></div><div>4. Better to raise a PETSC_ERR_SUP than to just leave this fragment in a comment<br><br>+    This code is only correct for the real case... Need to modify for complex numbers...<br>
<br><br>5. If this thing works (which putting it in the public interface usually indicates) then it needs a man page.<br><br><div>--- a/src/ksp/ksp/interface/iterativ.c</div><div>+++ b/src/ksp/ksp/interface/iterativ.c</div>
<div>@@ -330,6 +330,42 @@</div><div>   PetscFunctionReturn(0);</div><div> }</div><div> </div><div>+#undef __FUNCT__</div><div>+#define __FUNCT__ "KSPMonitorDynamicTolerance"</div><div>+/*</div><div>+ A hack to using dynamic tolerence in preconditioner</div>
<div>+ */</div><div>+PetscErrorCode KSPMonitorDynamicTolerance(KSP ksp,PetscInt its,PetscReal fnorm,void *dummy) {</div></div><div><br><br>6. Two different new features and no tests!<br><br><br><br></div><div>I'm a little concerned that the current review process is all ex post facto. It makes more work for reviewers and the history ends up looking confusing. I think we should work out a review process that can happen before things are pushed to petsc-dev, so that the authors of the patches can clean them up in response to comments. When done right, this ends up being less work for both the reviewers and the authors.</div>