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 <[EMAIL PROTECTED]> _______________________________________________ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev