[petsc-dev] Patch review

[Jie Chen]

I am in fact somewhat reluctant to push out everything before it is formally published and fully tested. The patch consists of more than one algorithmic developments that may be far away from maturity (but they are likely to work), though the authors might be the only ones who care about the new algorithms at this point. Besides, the codes are now full of hacks and lack documentation. I have no problem reverting petsc to the old version temporarily, and I promise I will clean everything to meet the production requirement, although this might not happen in a very short time. Meanwhile I think it also does not hurt to keep the patch as is, as the modification is likely to be used by the author circle only.

Jie




On 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/998bf52f/attachment.html>