Cleaned all except 5. Pushed to petsc-dev.<div>Hong<br><br><div class="gmail_quote">On Fri, Aug 24, 2012 at 5:42 PM, Jed Brown <span dir="ltr"><<a href="mailto:jedbrown@mcs.anl.gov" target="_blank">jedbrown@mcs.anl.gov</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><a href="http://petsc.cs.iit.edu/petsc/petsc-dev/rev/067c5dbc352e" target="_blank">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>

</blockquote></div><br></div>