t.rast.univar, t.rast3d.univar: Add support for zones #2588
Conversation
ninsbl
added
enhancement
ninsbl
requested review from
lucadelu,
neteler,
wenzeslaus and
NikosAlexandris
|
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... |
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.
Sounds reasonable to me. Are there use cases in which zones might be float? Time series of zones would be cool too... |
|
Thanks for all the testing you did (also on addons!). |
|
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. |
The point of such a check would be mostly to catch a user error, as |
|
Thanks for the thorough review and feedback. Will address this later today. |
|
Hm... Any Ctypes import seems to break the CentOS 7 build... |
|
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 |
I would be reluctant to remove CentOS just because one module failure to build when the other modules work.
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. |
We could use AlmaLinux: |
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... |
|
OK to merge? With this in, I can take on parallelization... |
There was a problem hiding this comment.
Thank you for addressing my concerns. The code is agreeable in general and it has tests. I did not review the functionality or logic.
If at least one of the other suggested reviewers reviews the logic or tests, that would be great. @lucadelu @neteler @NikosAlexandris @veroandreo
|
@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... |
|
@ninsbl sorry but in this period I have really no time to do this |
|
@lucadelu , fully understand. Given that the module is covered by tests and results in tests are unchanged, I merge the change. |
* add support for zones * add support for zones * add test for zones * clean properly * add test for zones * add support for zones * add credits * add credits * add zones in manual * add check for zones map * black * use RasterRow context manager * fix zones existence test * try if centos fails because of context manager * fix indent * change import order * zones check for t.rast3d.univar * avoid RasterRow * avoid RasterRow * fix if * fix if * fix raster_info * fix raster_info * fix raster_info * fix raster_info * import as gs * import as gs and zones * string formating * clean test * add semantic labels * black * remove gscript * name tests * name tests * move parser, use kwargs * move parser, use kwargs, allow DCELL * Import tgis after parser * Import tgis after parser * Fix typo
* add support for zones * add support for zones * add test for zones * clean properly * add test for zones * add support for zones * add credits * add credits * add zones in manual * add check for zones map * black * use RasterRow context manager * fix zones existence test * try if centos fails because of context manager * fix indent * change import order * zones check for t.rast3d.univar * avoid RasterRow * avoid RasterRow * fix if * fix if * fix raster_info * fix raster_info * fix raster_info * fix raster_info * import as gs * import as gs and zones * string formating * clean test * add semantic labels * black * remove gscript * name tests * name tests * move parser, use kwargs * move parser, use kwargs, allow DCELL * Import tgis after parser * Import tgis after parser * Fix typo
wenzeslaus
changed the title
* add support for zones * add support for zones * add test for zones * clean properly * add test for zones * add support for zones * add credits * add credits * add zones in manual * add check for zones map * black * use RasterRow context manager * fix zones existence test * try if centos fails because of context manager * fix indent * change import order * zones check for t.rast3d.univar * avoid RasterRow * avoid RasterRow * fix if * fix if * fix raster_info * fix raster_info * fix raster_info * fix raster_info * import as gs * import as gs and zones * string formating * clean test * add semantic labels * black * remove gscript * name tests * name tests * move parser, use kwargs * move parser, use kwargs, allow DCELL * Import tgis after parser * Import tgis after parser * Fix typo
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_labelsshould 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
zonesmap 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.