[GRASS-dev] Re: r.series, r.terraflow, simwe

[Glynn Clements]

Helena Mitasova wrote:

> (I am sending this off-list but feel free to post
> it with your answer if you think it should
> go to the developers list)
> 
> First of all thank you very much for your help with r.series,
> the paper where we have used it extensively got accepted
> without any need for revision and is now in press for Journal
> of Coastal Research.
> 
> I have a few questions related to recent changes in flow routing
> modules:
> 
> - Is the latest fix #197 submitted by Andy Danner for
> r.terraflow good enough now?

I can't comment on the fixes to the SFD support, as I don't understand
r.terraflow enough. However, it does resolve the issues which I had
with the original patch going to unnecessary lengths to eliminate
warnings.

> If yes, would you be so kind and submit

I'll do that as soon as I check that it still compiles (I haven't
checked since making all of the "struct Option" changes which were
triggered by this issue).

> it before Will moves r.terraflow into the iostream directory as
> suggested by Paul.

Can you provide details?

> - I haven't yet updated my trunk to the latest
> grass7 but I noticed that you say that simwe is
> severely broken. I have been using
> it a lot over the past 2 years and Yann has cleaned up the code
> a little bit few months ago so I am wondering whether we have
> messed it up or it has been broken from the start and I just
> don't use the broken options. The site's related code
> has not been updated properly so that may be causing
> problems (I can comment it out - it is not absolutely needed,
> although it is nice to have for outputs like this
> http://skagit.meas.ncsu.edu/~helena/wrriwork/balsam/fan
> http://skagit.meas.ncsu.edu/~helena/gmslab/gisc00/duality.html
> in addition to the raster series)

It is the sites code which is the problem.

I have recently fixed the lib/sites code to use "struct Map_info *"
for the "handle" to a sites "file", rather than continuing to pretend
that it's a "FILE *".

This change was mostly straightforward, but it did show up a couple of
issues with simwe. Specifically:

1. The code is confused about "fw". input_data() in simlib/input.c
opens it with fopen(), main_loop() in simlib/hydro.c writes textual
data to it with fwrite(), but main() in r.sim.sediment/main.c closes
it with G_sites_close() (which requires a "struct Map_info *").

2. fdoutwalk never gets closed. output_data() in simlib/output.c opens
it on demand, and that function gets called repeatedly from the
modules. When sites "files" were just files, they didn't need to be
explicitly closed, as all files are closed automatically upon exit. 
However, vector maps actually need to be closed (G_sites_close() calls
Vect_build()).

Point 1 probably isn't a problem; it appears that the G_sites_close()
call just needs to be changed to fclose() as is done by r.sim.water.

Point 2 is somewhat harder, due to:

> I know that the code is ugly

For this reason, I gave up trying to understand how to fix the code,
and just disabled the module.

> - as it is (similarly as v.surf.rst) a crude rewrite of our fortran
> code

My first thought on looking at the code was "this looks like fortran".

My second thought was "why does each module have its own copy of
waterglobs.h, when it's essential that both the library and the
modules have consistent definitions?"

> - but maybe with some help
> we can make it acceptable for keeping it in GRASS?

The fundamental problem with the code is the lack of modularity.

There are roughly 140 global variables (which are actually created by
each module and referenced by the library, which is backwards), yet
relatively few file-local variables and function arguments. This makes
it practically impossible to determine the consequences of any change
without analysing the entire code of the library and both modules.

At least a few of those variables are only used within a single
function (e.g. fdsfile is never used outside of input_data), so should
be local variables.

Ideally, a library should neither define nor use *any* global
variables[1]; all data should be passed and returned using function
arguments. If this is impractical due to the number of arguments[2],
the next best thing is to make variables local to a file containing
the functions which use them, or to bundle associated variables into a
structure (but don't over-do this, otherwise it just shifts the problem
from needing to check the entire source for variable references to
needing to check the entire source for structure field references).

[1] Although GRASS itself deviates from this ideal quite often, with
the end result that the libraries aren't much use for anything except
typical GRASS-style command-line modules.

[2] For an example of what isn't practical, see IL_init_params_2d(). I
have been seeing this warning for as long as I can remember:

main.c:643: warning: passing arg 41 of `IL_init_params_2d' from incompatible pointer type

Argument 41 is ... lets see ... one, two, three, fou... ah, never
mind, I'll just leave it.

-- 
Glynn Clements <glynn at gclements.plus.com>