Opened 5 years ago
Last modified 4 years ago
#3742 new enhancement
t.rast.aggregate: allow multiple methods/output
| Reported by: | sbl | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.8.3 |
| Component: | Temporal | Version: | svn-trunk |
| Keywords: | t.rast.aggregate | Cc: | |
| CPU: | All | Platform: | All |
Description
After r.series received support for multiple methods/outputs in one run, computing multiple STDS statistics with t.rast.aggregate could get a speed up if this is handed down to this module.
Compared to t.rast.series (#3741), the change is a bit more complex / comprehensive...
Attachments (3)
Change History (24)
comment:1 by , 5 years ago
| Milestone: | → 7.8.0 |
|---|
follow-up: 3 comment:2 by , 5 years ago
by , 5 years ago
| Attachment: | parser_patch.diff added |
|---|
Patch to support G_OPT_STRDS_OUTPUTS in parser
comment:3 by , 5 years ago
Concerning the last question:
Replying to sbl:
Finally, a new standard parser option would probably be nice like G_OPT_STRDS_OUTPUTS, as there is currently no standard option for multiple STDS outputs(just input), see: https://grass.osgeo.org/grass77/manuals/parser_standard_options.html
Attached a patch (attempt) to support G_OPT_STRDS_OUTPUTS in parser, pls check.
comment:4 by , 5 years ago
Please find attached a patch with a first attempt to allow for multiple methods (and thus output) in t.rast.aggregate. Testing is very welcome!
Unfortunately, I forgot to use your patch, Markus. Will update ASAP.
It is implemented as outlined above, where current behavior with regards to map names is kept, if only one method is requested. If however multiple methods are requested, the method name is injected between basename and suffix.
The patch also fixes one test for the temporal framework, which partly was due to quoting that was introduced for t.info... (Maybe a different ticket)
follow-up: 6 comment:5 by , 5 years ago
Hmm... With the tatch to support G_OPT_STRDS_OUTPUTS in parser applied, I get the following compilation error:
WARNING: Bug in UI description. Missing option key make: *** [../..//include/Make/Html.make:14: t.rast.aggregate.tmp.html] Error 1 rm t.rast.aggregate.tmp.html
when I use G_OPT_STRDS_OUTPUTS in t.rast.aggregate...
Any ideawhat could be wrong/missing?
follow-up: 8 comment:7 by , 5 years ago
Now I tried a completely new and fresh svn checkout. Yet, I still get the same error message when I use G_OPT_STRDS_OUTPUTS ...
follow-ups: 9 15 comment:8 by , 5 years ago
comment:9 by , 5 years ago
Replying to neteler:
Well, did you locally apply my patch?
Yes, I did. And I double-checked that changes were present in the code after I applied the patch and before I compiled...
However, now submitted to trunk in r74118 for the easy of solving this ticket.
Thanks, then, for testing your patch one only has to add an S to G_OPT_STRDS_OUTPUT...
comment:10 by , 5 years ago
Hmmm... Now I fetched fresh latest revision (r74196) from SVN, applied the changes to t.rast.aggregate.py and aggregation.py, but I still get the Error messsage above: WARNING: Bug in UI description. Missing option key...
I would very much like to get this into 7.8...
comment:11 by , 5 years ago
Can you please update the patch (replace the existing one attached here)?
Because it fails when applied to trunk:
patch -p0 < /tmp/t.rast.aggregate_multimethod.diff patching file lib/python/temporal/aggregation.py patching file temporal/t.rast.aggregate/t.rast.aggregate.py patching file temporal/t.rast.aggregate/testsuite/test_aggregation_absolute.py Reversed (or previously applied) patch detected! Assume -R? [n] Apply anyway? [n] Skipping patch. 5 out of 5 hunks ignored -- saving rejects to file temporal/t.rast.aggregate/testsuite/test_aggregation_absolute.py.rej
by , 5 years ago
| Attachment: | t.rast.aggregate_multimethod.diff added |
|---|
Support for multiple methods and output in one run
comment:12 by , 5 years ago
Thanks for looking into this. Attached a diff with G_OPT_STRDS_OUTPUTS. On my system it fails to compile. but with G_OPT_STRDS_OUTPUT it works fine. If it is causeing trouble, using a prefix could be an option as well..
comment:13 by , 5 years ago
I still cannot apply it.
--- lib/python/temporal/aggregation.py (revision 74053) +++ lib/python/temporal/aggregation.py (working copy) ...
Trunk is currently at r74221 - so your local copy is not up to date (IMHO)...
comment:14 by , 5 years ago
Sorry for the issues with the patch. Now I added a new one based on r74221. Tested it on a newly svn checkout and there it was applicable. Hope that helps with testing.
comment:15 by , 5 years ago
follow-up: 17 comment:16 by , 5 years ago
Thanks Markus, for your support. Just tried the latest revision and realized, that G_OPT_STRDS_OUTPUTS also changes the option name from "output" to "outputs". Would that be OK, or is that considered an API change?
Python scripts, like the testsuite, will fail with: "ParameterError: output is not a valid parameter."...
comment:17 by , 5 years ago
Replying to sbl:
Thanks Markus, for your support. Just tried the latest revision and realized, that G_OPT_STRDS_OUTPUTS also changes the option name from "output" to "outputs".
Well, it did not _change_ but it is an additional entry.
Would that be OK, or is that considered an API change?
In which sense?
Python scripts, like the testsuite, will fail with: "ParameterError: output is not a valid parameter."...
Please let me know where it fails exactly (or, where to see that).
comment:21 by , 4 years ago
| Milestone: | → 7.8.3 |
|---|
After I had a closer look at the t.rast.aggregate code, it seems that changes at library level would be required.
Amongst others here: https://trac.osgeo.org/grass/browser/grass/trunk/lib/python/temporal/aggregation.py#L288
I can try to implement that (and create a diff for a possible solution for you to review before I commit).
Main question would be how to keep API stable when implementing changes with regards to multiple output.
A possible option would be to use base name as is, if only one method is requested. And only if multiple methods are requested, the method gets injected in the raster map name (between base name and suffix/number). That would maintain current behavior but would open up for multiple output.
Finally, a new standard parser option would probably be nice like G_OPT_STRDS_OUTPUTS, as there is currently no standard option for multiple STDS outputs(just input), see: https://grass.osgeo.org/grass77/manuals/parser_standard_options.html
But this is something I cannot implement, and maybe it is not worth it if this is the only forseable case...