t.rast.univar / t.rast3d.univar: Add support for zones

[ninsbl]

trafficstars

Inspired by #1474, this PR adds support for a zones map in t.rast.univar and t.rast3d.univar, covered by tests.

Maybe also semantic_labels should be added to the output (if the STRDS contains some)? What do you think. Happy to add it if you consider it useful.

Should I add a check if the zones map is of type CELL?

Of course, I don` expect you all to review, but I would be happy about some feedback, esp. with regards to the two questions above.

[ninsbl]

P.S.: Like #1474, this implements only a single, static raster map for zones. That is probably the most common use case (at least it is for me). However, using another STRDS with a defined temporal relation could be interesting as well, but would be a different PR...

[veroandreo]

Inspired by #1474, this PR adds support for a zones map in t.rast.univar and t.rast3d.univar, covered by tests.

Maybe also semantic_labels should be added to the output (if the STRDS contains some)? What do you think. Happy to add it if you consider it useful.

I think it would be useful, especially for STRDS of multiple bands. However, we faced a problem recently when teaching at FOSS4G, if you estimate NDVI with t.rast.mapcalc using a STRDS of S2 b4 and b8, the inherited semantic label is b8 (Section 10 here)... if we then ask for univar stats, this will also inherit an incorrect semantic label. In any case, it would be a nice addition.

Should I add a check if the zones map is of type CELL?

Sounds reasonable to me. Are there use cases in which zones might be float?

Time series of zones would be cool too...

[ninsbl]

Thanks for all the testing you did (also on addons!). The semantic_labels and time series for zones are likely issues for different PRs. Will address the CELL-check ASAP.

[ninsbl]

STRDS as input for zones could probably be implemented like https://github.com/OSGeo/grass/blob/releasebranch_8_2/temporal/t.rast.aggregate.d

However, a more general question then is if this should be implemented as a separate module (e.g. t.rast.univar.ds) or as part of t.rast.univar. Both options have pros and cons, but I am leaning towards implementing that as a new module (if there isn't an add on already)...

[ninsbl]

Sounds reasonable to me. Are there use cases in which zones might be float?

The point of such a check would be mostly to catch a user error, as r.univar would refuse ro work.

[ninsbl]

Does anyone have an idea why the CentOS build fails to import RasterRow?

The error occurs when the manual is build. The RasterRow module seems rarely used in the core code-base outside of tests. So maybe this is another, more fundamental OS/distro specific issue surfacing here, as tests for CentOS only cover build and execution of the grass command?

[nilason]

According to the log the build first fails at:

Traceback (most recent call last):
  File "/github/home/anaconda3/lib/python3.7/ctypes/__init__.py", line 97, in CFUNCTYPE
    return _c_functype_cache[(restype, argtypes, flags)]
KeyError: (<class 'ctypes.c_int'>, (<class 'grass.lib.ctypes_preamble.String'>,), 1)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/__w/grass/grass/dist.x86_64-pc-linux-gnu/scripts/t.rast.univar", line 69, in <module>
    from grass.pygrass.raster import RasterRow
  File "/__w/grass/grass/dist.x86_64-pc-linux-gnu/etc/python/grass/pygrass/raster/__init__.py", line 19, in <module>
    import grass.lib.gis as libgis
  File "/__w/grass/grass/dist.x86_64-pc-linux-gnu/etc/python/grass/lib/gis.py", line 566, in <module>
    ('checker', CFUNCTYPE(UNCHECKED(c_int), String)),
  File "/github/home/anaconda3/lib/python3.7/ctypes/__init__.py", line 99, in CFUNCTYPE
    class CFunctionType(_CFuncPtr):
TypeError: item 1 in _argtypes_ passes a union by value, which is unsupported.
make[3]: *** [t.rast.univar.tmp.html] Error 1

also, as far as I can tell from the log, it is using Python 3.7.6:

...
The following NEW packages will be INSTALLED:
...
python             pkgs/main/linux-64::python-3.7.6-h0371630_2
...

which is infamously bad with ctypes (eg. https://trac.osgeo.org/grass/ticket/4018 or https://trac.osgeo.org/grass/ticket/3883).

[ninsbl]

Thanks @nilason for your swift reply! Now I changed the code so that RasterRow is no longer used. The CentOS issue remains, but does not surface here any more.

[veroandreo]

STRDS as input for zones could probably be implemented like https://github.com/OSGeo/grass/blob/releasebranch_8_2/temporal/t.rast.aggregate.d

However, a more general question then is if this should be implemented as a separate module (e.g. t.rast.univar.ds) or as part of t.rast.univar. Both options have pros and cons, but I am leaning towards implementing that as a new module (if there isn't an add on already)...

Well, yes, perhaps a separate module is the easiest. IIUC, t.rast.aggregate.ds aggregates a STRDS with the granularity of another STRDS, so it's indeed a good starting point for the temporal sampling part (the input STRDS would need to be sampled with the granularity of the zones STRDS), the next step would be the spatial aggregation to get the stats for the zones in each time.. There will be then methods for temporal and methods for spatial aggregation... Yeah, better in a separate module

[ninsbl]

Thanks for the thorough review and feedback. Will address this later today.

[ninsbl]

Hm... Any Ctypes import seems to break the CentOS 7 build... Is it worth keeping CentOS 7 tests in the light of that? If I understand correctly CentOS transitioned to CentOS Stream. Unfortunately, newer CentOS images, like Stream, are not available in GitHub CI out of the box...

[ninsbl]

Tests are passing for ubuntu: https://github.com/OSGeo/grass/pull/2588/checks#step:10:216 and https://github.com/OSGeo/grass/pull/2588/checks#step:10:222 However, as mentioned CetnOS build tests fail when importing tgis: https://github.com/OSGeo/grass/actions/runs/3206937539/jobs/5241279653#step:11:9830 That worked before, so failing tests may be due to the order of when the import occurs?

[wenzeslaus]

Any Ctypes import seems to break the CentOS 7 build... Is it worth keeping CentOS 7 tests in the light of that?

I would be reluctant to remove CentOS just because one module failure to build when the other modules work.

Unfortunately, newer CentOS images, like Stream, are not available in GitHub CI out of the box...

Neither is CentOS 7, we are running it using a Docker image. Are there no Docker images for Stream?

With this said, the CentOS was always on shaky ground. It is partially using conda to get acceptable dependencies, so it is a poor test of CentOS. I was hoping it will move to more pure CentOS, but that did not happen yet. However, it does catch things which are related to robustness in sub-optimal environments.

[neteler]

Neither is CentOS 7, we are running it using a Docker image. Are there no Docker images for Stream?

We could use AlmaLinux: https://hub.docker.com/_/almalinux

[ninsbl]

I would be reluctant to remove CentOS just because one module failure to build when the other modules work.

Fair enough. And with your suggestion, tests are no longer failing. So I agree, best to keep it. Maybe improve the CentOS test in future, e.g. with the docker containers @neteler suggested...

[ninsbl]

OK to merge? With this in, I can take on parallelization...

[ninsbl]

@lucadelu @neteler @NikosAlexandris @veroandreo if one of you has a chance to test, that would be very much appreciated? Otherwise, we can trust CI and merge the next days...

[lucadelu]

@ninsbl sorry but in this period I have really no time to do this

[ninsbl]

@lucadelu , fully understand. Given that the module is covered by tests and results in tests are unchanged, I merge the change.