[petsc-dev] Patch review

[Jed Brown]

http://petsc.cs.iit.edu/petsc/petsc-dev/rev/067c5dbc352e

Just a couple requests regarding this patch:

1. Please split independent functionality into separate patches. This does
three different things.

2. Please cut out extra whitespace like this and follow conventions about
not including args in the declarations.

@@ -273,6 +274,12 @@
 PETSC_EXTERN PetscErrorCode KSPMonitorDefault(KSP,PetscInt,PetscReal,void
*);
 PETSC_EXTERN PetscErrorCode
KSPLSQRMonitorDefault(KSP,PetscInt,PetscReal,void *);
 PETSC_EXTERN PetscErrorCode KSPMonitorRange(KSP,PetscInt,PetscReal,void *);
+
+
+PETSC_EXTERN PetscErrorCode KSPMonitorDynamicTolerance(KSP ksp,PetscInt
its,PetscReal fnorm,void *dummy);
+PETSC_EXTERN PetscErrorCode KSPMonitorDynamicToleranceDestroy(void
**dummy);
+
+

3. This doesn't look like an improvement:

@@ -163,7 +163,7 @@
     /* scalar updates */
     omega = xi2 / xi3;
     beta = - xi4 / sigma;
-    rho = PetscSqrtReal(PetscAbsScalar(xi1 - omega * xi2)); /* residual
norm */
+    rho = sqrt(fabs(xi1 - omega * xi2)); /* residual norm */

     /* vector updates */


4. Better to raise a PETSC_ERR_SUP than to just leave this fragment in a
comment

+    This code is only correct for the real case... Need to modify for
complex numbers...


5. If this thing works (which putting it in the public interface usually
indicates) then it needs a man page.

--- a/src/ksp/ksp/interface/iterativ.c
+++ b/src/ksp/ksp/interface/iterativ.c
@@ -330,6 +330,42 @@
   PetscFunctionReturn(0);
 }

+#undef __FUNCT__
+#define __FUNCT__ "KSPMonitorDynamicTolerance"
+/*
+ A hack to using dynamic tolerence in preconditioner
+ */
+PetscErrorCode KSPMonitorDynamicTolerance(KSP ksp,PetscInt its,PetscReal
fnorm,void *dummy) {


6. Two different new features and no tests!



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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20120824/31130f84/attachment.html>