Re: [CHANGESET] gmres (Re: Last review of PROJECTS file before release)

[c.]

On 10 Feb 2011, at 18:56, John W. Eaton wrote:
> 
> Here are some very minor comments.  I'd say it is OK to push these
> changes now, but would prefer if you addressed the comments before
> pushing rather than after.
> 
> | @@ -188,3 +188,5 @@
> |  @DOCSTRING(bicgstab)
> |  
> |  @DOCSTRING(cgs)
> | +
> | address@hidden(gmres)
> 
> Is there some reason to not document mgorth in the manual?

no, I just didn't know where the right place to put it was, what would you 
suggest?
BTW, I see 'pcg' is in the section about sparse matrices rather than in the 
'specialized solvers' subsection of 'linear algebra'
I see no reason why pcg should be treated differently than bicgstab, cgs and 
gmres, wouldn't it make sense to move them all together? where?

> | +## Copyright (C) 2009,2010,2011 Carlo de Falco
> 
> To be consistent with other files in Octave now, please write
> 2009-2011.  That will make it easier for us to update this
> automatically in the future.

OK

> | +##  This program is free software; you can redistribute it and/or modify
> | +##  [...]
> | +##  along with this program; if not, write to the Free Software
> | +##  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 
> 02110-1301 USA
> 
> Please update this to use the statement for Octave. It says "This file
> is part of Octave" and ends with a URL instead of a physical address.

OK

> | +## @deftypefn {Function File} @
> | +##   address@hidden, @var{flag}, @var{relres}, @var{iter}, @var{resvec}] 
> =} pgmres (@var{A}, @var{b}, @var{m}, @var{rtol}, @var{maxit}, @var{M1}, 
> @var{M2}, @var{x0})
> | +##
> | +##   [x, resids] = pgmres (A, b, x0, rtol, maxit, m, P)
> 
> Shouldn't this line use deftypefnx and have some markup for the
> variable names?

that was just a leftover from the octave-forge version of the function I have 
removed it as suggested by Ben.

> | +%!shared A, b, dim
> | +%!test
> | +%! dim = 100;
> | +%! A = spdiags ([-ones(dim,1) 2*ones(dim,1) ones(dim,1)], [-1:1], dim, 
> dim);
> | +%! b =  ones(dim, 1); 
> | +%! x = gmres (A, b, 10, 1e-10, dim, @(x) x./diag(A), [],  b);
> | +%! assert(x, A\b, 1e-9*norm(x,inf))
> 
> Thanks for including tests!
> 
> | +              retval(0) = x;
> | +              retval(1) = h;
> 
> Please write
> 
>  retval(1) = h;
>  retval(0) = x;

OK

> Writing in the reverse order means that the array that is used to
> implement the octave_value_list object is only resized once rather
> than twice.
> 
> jwe

I'll submit another patch shortly...
c.