r.geomorphon: separate input from output #1052
Conversation
|
One of the checks had timed out after 6 hours. |
raster/r.geomorphon/main.c
Outdated
| for (i = 1; i < io_size; ++i) { /* WARNING: loop starts from one, zero is for input */ | ||
| for (i = o_forms; i < io_size; ++i) { /* WARNING: loop starts from one, zero is for input */ |
There was a problem hiding this comment.
Can you please add a comment that opt_output[0] is unused? The current comment does not necessarily imply that. Perhaps to the definition of opt_output. If my understanding isn't correct, even more reasons comment on this more.
There was a problem hiding this comment.
The comment at the first for loop says that, but not at the subsequent loops. opt_output[0] is accessed outside of the for loops, although to a questionable effect, so the next commit removes the element from the array. Let me push an updated version to see.
There was a problem hiding this comment.
Thanks, I didn't mean to submit the review yet and meant to delete this comment. The subsequent commit resolved this. I don't know if you needed to update anything.
raster/r.geomorphon/main.c
Outdated
| opt_input = G_define_standard_option(G_OPT_R_ELEV); | ||
| opt_input->key = rasters[0].name; | ||
| opt_input->key = "elevation"; | ||
| opt_input->required = YES; | ||
| opt_input->description = _(rasters[0].description); | ||
| opt_input->description = _("Name of input elevation raster map"); |
There was a problem hiding this comment.
You can actually drop key and description here. See https://grass.osgeo.org/grass78/manuals/parser_standard_options.html It is the same.
What you did is much better from GRASS GIS point of view than the previous implementation. (Standardization of interface between modules is more important than something (which looks) smart in one particular module, although that is not to say that the lib is perfect.)
There was a problem hiding this comment.
You are right. And required is excess as well.
There was a problem hiding this comment.
Right, esp. because elevation is the only input, so there is no need to be explicit about elevation being required.
The for loops that iterate over opt_output[] started from hard-coded 1 (o_forms), which is the first output, but not the first element in the array. Use the enumeration constant to iterate properly regardless of the amount of any preceding items in the array. Remove a comment that no longer applies.
All the output rasters were hard-coded as not required regardless of what was in the array, and the only input raster is a standard option that is required by default, so just remove the field from the structure.
The input raster element did not belong to the array because it used only two structure fields to store string constants that were excess for a standard option anyway; it also created a place for an off-by-one error in every loop over the array. Remove the element from the array, merge the single-use type into the only declaration that needs it and constify some fields. Rename io_size to o_size, remove i_dem, UNKNOWN and a stale comment.
|
Thank you for the review. Here is a new revision, which should address the points you have raised. |
|
It would be great to have this merged so I can rebase and open the next pull request with the remaining code cleanups. |
|
I take liberty to merge it |
These three commits remove some excess code in a way that should be easy to review. It would be fine to squash these changes together.