[petsc-dev] Patch review

[Hong Zhang]

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>