rsbivand / rgrass

Interpreted interface between the GRASS geographical information system and R
https://rsbivand.github.io/rgrass/
24 stars 8 forks source link

Faulty parameter rejection in execGRASS #83

Closed TillF closed 3 weeks ago

TillF commented 6 months ago

Fromm issue #82, I used rgrass version 0.41. Opposed to my prior version (0.38) where everything worked as expected, the following command now failed:

execGRASS("r.water.outlet", input="drain_t", output="basin_23", coordinates=c(1,2), intern = T)
  #Parameter <coordinates> has multiple values

Also, alternatively passing the coordinate pair as a string is rejected:

  execGRASS("r.water.outlet", input="drain_t", output="basin_23", coordinates=paste0(c(1,2), collapse=","), intern = T)
  #Fehler in doGRASS(cmd, flags = flags, ..., parameters = parameters, echoCmd = echoCmd,  : 
  #                    Parameter <coordinates> does not have numeric value

I would expect the first option to be the correct one. Please correct me if I am mistaken.

veroandreo commented 6 months ago

As you are writing a GRASS command in R, I'd write coordinates="1,2", as GRASS would take it. Did you try that one?

TillF commented 6 months ago

coordinates="1,2" corresponds to the second version I tried above (paste0(c(1,2), collapse=","))and give the respective error as not being numeric.

rsbivand commented 6 months ago

@TillF In general, I cannot reproduce anything without having access to your LOCATION. Either use initGRASS to create a throw-away LOCATION, or use nc_basic_spm_grass7. Here, however:

> doGRASS("r.water.outlet", input="drain_t", output="basin_23", coordinates=c(1,2))
Error in doGRASS("r.water.outlet", input = "drain_t", output = "basin_23",  : 
  Parameter <coordinates> has multiple values

The problem is in the GRASS program itself, as for GRASS 8.3.1:

    <parameter name="coordinates" type="float" required="yes" multiple="no">
        <description>
            Coordinates of outlet point
        </description>
        <keydesc>
            <item order="1">east</item>
            <item order="2">north</item>
        </keydesc>

and:

> parseGRASS("r.water.outlet")
Command: r.water.outlet 
Description: Creates watershed basins from a drainage direction map. 
Keywords: raster, hydrology, watershed 
Parameters:
  name: input, type: string, required: yes, multiple: no
  keydesc: name, keydesc_count: 1
[Name of input drainage direction map]
  name: output, type: string, required: yes, multiple: no
  keydesc: name, keydesc_count: 1
[Name for output watershed basin map]
  name: coordinates, type: float, required: yes, multiple: no
  keydesc: eastnorth, keydesc_count: 1
[Coordinates of outlet point]
Flags:
  name: overwrite [Allow output files to overwrite existing files] {FALSE}
  name: help [Print usage summary] {FALSE}
  name: verbose [Verbose module output] {FALSE}
  name: quiet [Quiet module output] {FALSE}

Since I don't have your LOCATION, I can't try system("r.water.outlet input=drain_t output=basin_23 coordinates=1,2", intern=TRUE) to see whether GRASS itself requires adherence to no-multiples - in nc_basic_spm_grass7 it does not, but does not declare the parameter as taking multiple values in code. Specifically, adding opt.coords->multiple = YES; after line 70 in raster/r.water.outlet/main.c removes the problem:

> parseGRASS("r.water.outlet")
Command: r.water.outlet 
Description: Creates watershed basins from a drainage direction map. 
Keywords: raster, hydrology, watershed 
Parameters:
  name: input, type: string, required: yes, multiple: no
  keydesc: name, keydesc_count: 1
[Name of input drainage direction map]
  name: output, type: string, required: yes, multiple: no
  keydesc: name, keydesc_count: 1
[Name for output watershed basin map]
  name: coordinates, type: float, required: yes, multiple: yes
  keydesc: eastnorth, keydesc_count: 1
[Coordinates of outlet point]
Flags:
  name: overwrite [Allow output files to overwrite existing files] {FALSE}
  name: help [Print usage summary] {FALSE}
  name: verbose [Verbose module output] {FALSE}
  name: quiet [Quiet module output] {FALSE}

@veroandreo could you please raise an issue in GRASS that for an unknown number of programs, the behaviour of the programs differs from the interface description where the man page and the program want a multiple that is not emitted in the interface description, example r.water.outlet ?

TillF commented 6 months ago

Apologies, I had not bothered to give more details on my specific GRASS-settings and data, as the command seems to be rejected before even reaching GRASS.

system("r.water.outlet input=drain_t output=basin_23 coordinates=1,2", intern=TRUE) yields

Fehler in system("r.water.outlet input=drain_t output=basin_23 coordinates=1,2",  : 
  'r.water.outlet' not found

as GRASS is not in my PATH variable.

r.water.outlet input=drain_t output=basin_23 coordinates=1,2 works as expected when executed from the GRASS-shell (i.e. in the console after having started GRASS).

rsbivand commented 6 months ago

This is expected. Start R from the GRASS console.

TillF commented 6 months ago

Having started R from the GRASS console, and running initGRASS with my specs still yields the same result for the system()-call: Fehler in system("r.water.outlet input=drain_t output=basin_23 coordinates=1,2", : 'r.water.outlet' not found

rsbivand commented 6 months ago

Can't help with this. You could re-install rgrass 0.3-9 from CRAN, which appears to ignore the same no-multiples rule.

TillF commented 6 months ago

Reinstalled 0.3-9, solves the issue for me. Apologies, but does this mean I have to freeze at this version? I don't quite understand the details you give above. Is this a GRASS issue, or something specific to my setting? I guess it is not related to not being able to run the GRASS-command via system(), right?

rsbivand commented 6 months ago

@TillF You have encountered a problem in GRASS that when a specified number of values greater than 1 is needed, the interface description is ambiguous. In this case, multiple coordinates are not permitted (as optionally multiple buffer distances in r.buffer would be), but more than one value is required.

In 0.3-9 (using the XML package to convert the interface description to a format rgrass can handle), this worked. The XML package is end-of-life, so 0.4-1 replaces it with the xml2 package. In the new code, the workaround at https://github.com/rsbivand/rgrass/blob/07429b4f2480bd7bfe2f09d52f79f27e977b681d/R/xml1.R#L231-L236 does not (yet) work, as the normally unneeded keydesc_count is parsed incorrectly. So your report is useful in stress-testing the transition from XML to xml2.

@veroandreo we don't need to alarm GRASS devs in this case, as multiple should be NO, but I need to find out why keydesc_count is wrong.

rsbivand commented 6 months ago

There was an unnoticed difference between XML and xml2 in handling multiple items to construct keydesc_count. New source package, Windows binary to follow: rgrass_0.4-1.tar.gz

rsbivand commented 6 months ago

Windows binary: rgrass_0.4-1.zip

@TillF Please let us know if this resolves the problem.

florisvdh commented 3 weeks ago

@TillF can you confirm this issue is solved?

With rgrass 0.4.2 we have:

> doGRASS("r.water.outlet", input="drain_t", output="basin_23", coordinates=c(1,2))
[1] "r.water.outlet input=drain_t output=basin_23 coordinates=1,2"
attr(,"cmd")
[1] "r.water.outlet"
> parseGRASS("r.water.outlet")
Command: r.water.outlet 
Description: Creates watershed basins from a drainage direction map. 
Keywords: raster, hydrology, watershed 
Parameters:
  name: input, type: string, required: yes, multiple: no
  keydesc: name, keydesc_count: 1
[Name of input drainage direction map]
  name: output, type: string, required: yes, multiple: no
  keydesc: name, keydesc_count: 1
[Name for output watershed basin map]
  name: coordinates, type: float, required: yes, multiple: no
  keydesc: east,north, keydesc_count: 2
[Coordinates of outlet point]
Flags:
  name: overwrite [Allow output files to overwrite existing files] {FALSE}
  name: help [Print usage summary] {FALSE}
  name: verbose [Verbose module output] {FALSE}
  name: quiet [Quiet module output] {FALSE}
TillF commented 3 weeks ago

works for me, thanks.