Cleaned all except 5. Pushed to petsc-dev. Hong On Fri, Aug 24, 2012 at 5:42 PM, Jed Brown <jedbrown at mcs.anl.gov> wrote:
> 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/f22bb3ca/attachment.html>
