r.geomorphon: combine profile parser rules to allow usage as pygrass module

[ninsbl]

parser rules as they were written before this PR, did not allow to construct a pygrass Module object for r.geomorphon in a straightforward fashion. In the testsuite the Module is build and run from a command string, which is not the most common approach when doing scripting. When constructing the Module object "manually", the profileformat option cannot be set to "nothing" and thus requires coordinates which may not be desired.

This small PR attempts to fix this.

If no profiledata output is requested, default values in proileformat are disregarded. API should be unchanged.

@infrastation, do you think it is OK this way?

In Pygrass Module, one still has to specify profiledata="", which is not very intuitive... Maybe the default answer for profiledatashould be removed too. Ideas and oponions welcome...!

Not sure if it needs backporting to 7.8...

[ninsbl]

Maybe rather this way: coorinates option triggers requirements to define profile settings (which both keep their defaults)

[infrastation]

See also #2029. Using the representation from #1157 (comment), below is the tree of the currently implemented options:

* REQ: "elevation"
* REQ: exactly one of:
	* OPT: (raster mode), implicit, one or more of:
		* OPT: "forms"
		* OPT: "ternary"
		* OPT: "positive"
		* OPT: "negative"
		* OPT: "intensity"
		* OPT: "exposition"
		* OPT: "range"
		* OPT: "variance"
		* OPT: "elongation"
		* OPT: "azimuth"
		* OPT: "extend"
		* OPT: "width"
	* OPT: (one-off mode) "coordinates", enables:
		* OPT: "profiledata" (has a default value)
		* OPT: "profileformat" (has a default value)
* OPT: "search" (has a default value)
* OPT: "skip" (has a default value)
* OPT: "flat" (has a default value)
* OPT: "dist" (has a default value)
* OPT: "comparison" (has a default value)
* OPT: "-m"
* OPT: "-e"

In plain language, regardless of the option names there are two data processing modes. Less the command line options and flags, both take raster data as input (hence "elevation" is always required). One mode produces a set of rasters as output data. The other produces machine-readable text and requires two other parameters (whether with a default value, or without), which have no purpose in the first mode.

As a side note, the original motivation for making these two modes mutually exclusive was to complicate the raster iteration loops in the C code as little as possible. With some careful code reworking it might be possible to implement any mixture of the outputs (rasters and/or machine-readable profile data), but it is difficult to see a valid use case for that, and even in that case it would be necessary to keep the "profiledata" and "profileformat" options conditional. So let's keep the two modes mutually exclusive for now.

The proposed change replaces "both profiledata and profileformat require coordinates" with "coordinates requires both profiledata and profileformat". Please note that the one-off mode in principle requires all three options, which one is the trigger for this mode and whether the other two options have a default value is an implementation detail. AFAIR, in early draft versions "profiledata" was the trigger, but after it started to have a default value "coordinates" became one. This way, the proposed change should be good if the default values in "profiledata" and "profileformat" satisfy the parser expectations. The following test shows that this is not the case:

r.geomorphon elev=aw3d_dem_30m coordinates=354000,651000
[...]
ERROR: Option <coordinates> requires at least one of <profiledata> or <profileformat>

Without this change the parser uses the defaults and the same command prints JSON to standard output. So this particular change is not a valid solution. That said, I accept a different solution might be required to address a problem in at least one of the following areas:

• parser documentation, which might not specify the semantics of default, optional and conditional precisely enough
• parser C code, which might implement not entirely what the documentation specifies or should specify
• pygrass Python code, which might assume something a bit different from the C implementation

As far as I understand, at this time the problem mostly manifests in pygrass. I have not used it before, but if you state the problem in more detail and tell how to reproduce it, I might have some input. There is also a possible workaround of falling back to DIY semantic checks in the C code instead of proper parser declarations. Let me know what works better.

[ninsbl]

Thanks @infrastation for checking and commenting on this.

Indeed, the one-off mode did not work after my suggested changes. I wrongly assumed, the parser would take the default as an answer.

Given that defaults are defined, the one-off mode should - as you correctly point out - work also if the user provides only coordinates and does not provide input for profiledata or profileformat (as defaults are used). Thus, removing the parser rule seems to be save from my point of view and may be the right solution. I tested your example and it works fine now... And the behavior of the module is clearly defined in that case (write JSON format to stdout).

However, maybe the error message if neither coordinates nor output are given should be changed from:
ERROR: At least one output is required, e.g. forms to e.g.:
ERROR: Either at least one output (e.g. forms) or coordinates are required.

As for pygrass: I think, what pygrass does is taking the defaults from the --interface-description of a module (see: https://grass.osgeo.org/grass78/manuals/libpython/pygrass.modules.interface.html#module-pygrass.modules.interface.module) and fills in defaults. That may be a bug in pygrass, but it is a different issue I would say...

[infrastation]

Thank you for the update. Let me take some time to think about it.

[ninsbl]

@infrastation did you find the time to look at this? It would be really great to get r.geomorphon working in PyGRASS in 8.0.1 as I need that in a project where I have to share a script and I cannot expect my colleagues to patch and compile their GRASS installations ...

[infrastation]

Please excuse me, I was busy with other work. Regarding the message text, I agree it would be more useful reworded as you suggest. Regarding the main problem, as of now the proposed change looks as follows:

--- a/raster/r.geomorphon/main.c
+++ b/raster/r.geomorphon/main.c
@@ -183,7 +183,6 @@ int main(int argc, char **argv)
         par_profiledata->description =
             _("Profile output file name (\"-\" for stdout)");
         par_profiledata->guisection = _("Profile");
-        G_option_requires(par_profiledata, par_coords, NULL);
 
         par_profileformat = G_define_option();
         par_profileformat->key = "profileformat";
@@ -193,7 +192,6 @@ int main(int argc, char **argv)
         par_profileformat->required = NO;
         par_profileformat->description = _("Profile output format");
         par_profileformat->guisection = _("Profile");
-        G_option_requires(par_profileformat, par_coords, NULL);
 
         if (G_parser(argc, argv))
             exit(EXIT_FAILURE);

From the user's point of view this means that it makes profiledata and profileformat valid arguments even without coordinates (no-op in this case), which does not look right:

> r.geomorphon elev=aw3d_dem_30m forms=testforms profileformat=yaml profiledata=test.yaml
 100%
r.geomorphon complete.

So it would replace one parsing issue with another.

To understand the problem better, I had a look at the --interface-description XML of the plugin as it is now, and the <rules> element content looks consistent with the current C code, which looks consistent with the semantic description above (REQ/OPT). If there is a problem at the sending end of the XML feed, it is not so apparent, but it is apparent at the receiving end:

>>> from grass.pygrass.modules import Module
>>> geom = Module ('r.geomorphon')
>>> geom.get_bash()
'r.geomorphon search=3 skip=0 flat=1.0 dist=0.0 comparison=anglev1 profileformat=json profiledata=-'

So a correct long-term fix most likely belongs to pygrass space.

That said, a short-term workaround (if the problem trigger is the the default values) could be removing the defaults for profiledata and profileformat; that would make the user interface a bit less convenient, but would avoid the erroneous code path downstream:

--- a/raster/r.geomorphon/main.c
+++ b/raster/r.geomorphon/main.c
@@ -178,22 +178,22 @@ int main(int argc, char **argv)
 
         par_profiledata = G_define_standard_option(G_OPT_F_OUTPUT);
         par_profiledata->key = "profiledata";
-        par_profiledata->answer = "-";
         par_profiledata->required = NO;
         par_profiledata->description =
             _("Profile output file name (\"-\" for stdout)");
         par_profiledata->guisection = _("Profile");
         G_option_requires(par_profiledata, par_coords, NULL);
+        G_option_requires(par_coords, par_profiledata, NULL);
 
         par_profileformat = G_define_option();
         par_profileformat->key = "profileformat";
         par_profileformat->type = TYPE_STRING;
         par_profileformat->options = "json,yaml,xml";
-        par_profileformat->answer = "json";
         par_profileformat->required = NO;
         par_profileformat->description = _("Profile output format");
         par_profileformat->guisection = _("Profile");
         G_option_requires(par_profileformat, par_coords, NULL);
+        G_option_requires(par_coords, par_profileformat, NULL);
 
         if (G_parser(argc, argv))
             exit(EXIT_FAILURE);

This version seems to handle the options consistently , this is what it produces:

>>> from grass.pygrass.modules import Module
>>> geom = Module ('r.geomorphon')
>>> geom.get_bash()
'r.geomorphon search=3 skip=0 flat=1.0 dist=0.0 comparison=anglev1'

What do you think about it?

[ninsbl]

What do you think about it?

Thanks @infrastation for bearing with me here. Very much appreciated! Your suggestion sounds very reasonable to me, so I implemented it in 6a02309.
If there are no objections from others I will merge the change (after and if tests pass).
Thanks again.

[infrastation]

You are welcome. The HTML file would need to be updated as well.

[wenzeslaus]

I think you should be able to re-enable the r.geomorphon test now:

https://github.com/OSGeo/grass/blob/main/.gunittest.cfg#L20

...which I missed when we introduced this issue.

[ninsbl]

Manual updated in 13819f1 and test reactivated in ba10857