[Developers] Re: patch: CactusEinstein/IDAxiBrillBH cleanup

[Tom Goodale]

It all looks good, please go ahead, although as you note at the bottom it 
would be good if you could split the commits into smaller units, each 
addressing an individual thing, e.g.

      relabeling latex
      adding comments to param.ccl
      adding the interpolator parameters
      adding new output (I agree that it should really be a GF so we can
                         use standard Cactus IO methods)
      add debugging stuff
      adding new stuff to the documentation
      etc

In general, small, specific patches will get quicker attention, and thus 
lower latency, than big patches which do lots of different things.  If 
you'd sent a note out to developers that you were doing this, and followed 
with a sequence of patches as you did things, it would have been easier.

Thanks for doing all this.

Tom

On Fri, 20 May 2005, Jonathan Thornburg wrote:

> [[I originall sent this patch to developers@ on 9.May.2005, but
> the mailing list seems to have somehow lost it -- noone else seems
> to have gotten it, and it's not in the web mailing list archives.
> Hence this resend...]]
>
>
> Hi,
>
> Over the past month or so I've been working on a general cleanup of
> CactusEinstein/IDAxiBrillBH, and this is now in a state where I'd
> like to commit the changes described by the enclosed patch, to CVS.
>
> The changes affect 5 files in this thorn:
>
> param.ccl
> * add comments
> * add additional interpolator parameters (described in detail below)
> * add "output psi on 2D grid" parameters (described in detail below)
> * add debugging parameters
>
> doc/documentation.tex
> * add "IDAxiBrillBH/" prefix to make LaTeX labels unique across thorns
> * correct a few typos
> * clarify explanation of how we solve on a 2-D grid and then interpolate
>  to the Cactus 3-D grid
> * explain the new interpolator and output-psi-on-2D-grid parameters
> * add some comments on choosing the 2-D grid resolution parameters
> * mention debug parameters
>
> doc/TODO
> * new file noting some problems with this thorn which I found, but
>  didn't fix
>
> src/IDAxiBrillBH.F
> * use new CCTK_WARN(CCTK_WARN_ABORT, ...) instead of old CCTK_WARN(0, ...)
> * more flexible interpolator parameters:
> 	This thorn first solves the Brill-wave equation on an internal
> 	(axisymmetric) 2D grid, then uses uses the standard Cactus
> 	CCTK_InterpLocalUniform() local-interpolator API to interpolate
> 	this to the 3D grid points.  Before this patch, this thorn
> 	hard-wired the interpolation operator to "uniform cartesian",
> 	only allowed orders 1, 2, and 3, and didn't allow any other
> 	interpolator parameters to be set.  This patch adds two new
> 	parameters to allow any interpolation operator and parameters
> 	to be specified.  Either the old or the new parameters can be
> 	used (see doc/documentation.tex for details of how this works);
> 	the defaults leave the behavior of this thorn unchanged.
> * add option to output psi on 2D grid
> 	When using this thorn, I found it hard to choose the parameters
> 	defining the resolution and extent of the internal 2D grid.
> 	To help with this, I added an option to output the solution
> 	on that grid to an ASCII data file, so it can be examined
> 	and plotted.
> * add debugging code
> 	I've added debug options which, if enabled, print the values
> 	of various internal quantities at specified 2D and/or 3D grid
> 	points.  I've left these in the final "production" code as
> 	possibly being useful for future debugging.
> * add many comments
> * correct or remove some old comments which were out of date
> * adjust whitespace to make the code a bit more readable:
>  I've changed code like
>       allocate(cc(neb,nqb),ce(neb,nqb),cw(neb,nqb),cn(neb,nqb),cs(neb,nqb),
>      $     rhs(neb,nqb),psi2d(neb,nqb),detapsi2d(neb,nqb),dqpsi2d(neb,nqb),
>      $     detaetapsi2d(neb,nqb),detaqpsi2d(neb,nqb),dqqpsi2d(neb,nqb),
>      $     etagrd(neb),qgrd(nqb))
>  to make it clearer which arrays have the same size:
>       allocate(          cc(neb,nqb))
>       allocate(          ce(neb,nqb))
>       allocate(          cw(neb,nqb))
>       allocate(          cn(neb,nqb))
>       allocate(          cs(neb,nqb))
>       allocate(         rhs(neb,nqb))
>       allocate(       psi2d(neb,nqb))
>       allocate(   detapsi2d(neb,nqb))
>       allocate(     dqpsi2d(neb,nqb))
>       allocate(detaetapsi2d(neb,nqb))
>       allocate(  detaqpsi2d(neb,nqb))
>       allocate(    dqqpsi2d(neb,nqb))
>
>       allocate(etagrd(neb))
>       allocate(  qgrd(nqb))
> * change some Fortran write statements to CCTK_INFO
> 	[so they're properly synchronized with output from C routines,
> 	even when stdout+stderr are redirected to a log file]
> * include more information about what went wrong, in various error messages
> 	[eg if the interpolator returns an error code, the code
> 	now includes that error code in the error message]
> * rename a few variables to make their purpose clearer:
> 	ep1 --> error_at_this_grid_point
> 	ep2 --> max_error_in_grid
>
> src/interp2.F --> archive/interp2.F
> * this file is unused; move it from src/ to new directory archive/
> 	[I chose to keep it in archive/, rather than only in the
> 	CVS attic, because the CVS attic isn't usable with normal
> 	Unix tools ("cat, awk, and grep" et al).]
>
>
>
> Finally, a metacomment about patch granularity:
>
> As you can see, this patch contains a whole set of changes.  In an
> ideal world, each change would be a separate patch.  In practice,
> though, splitting things up this way is quite awkward -- each patch
> has its own patch-review-by-Tom latency, and until patch N is committed
> to CVS, it's cumbersome to generate the diffs for patch N+1.  Committing
> my changes immediately to a private CVS branch would solve that problem,
> but as I understand it, even on a private CVS branch, commits would
> still need Tom's approval.
>
> Apart from "switch to darcs", is there an elegant solution to this
> problem?
>
>
> ciao,
>
>