t.rast.aggregate: allow multiple methods/output

[sbl]

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...

[sbl]

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...

[neteler]

Patch to support G_OPT_STRDS_OUTPUTS in parser

[neteler]

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.

[sbl]

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)

[sbl]

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?

[neteler]

Did you run "make distclean" prior to compiling the source code?

[sbl]

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 ...

[neteler]

Replying to sbl:

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 ...

Well, did you locally apply my patch?

However, now submitted to trunk in r74118 for the easy of solving this ticket.

[sbl]

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...

[sbl]

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...

[neteler]

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

[sbl]

Support for multiple methods and output in one run

[sbl]

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..

[neteler]

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)...

[sbl]

Revised patch

[sbl]

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.

[neteler]

Replying to neteler:

However, now submitted to trunk in r74118 for the easy of solving this ticket.

Argh, it was incomplete. Now fixed in r74222 (and your patch works here locally).

[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". 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."...

[neteler]

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).

[neteler]

Ticket retargeted after milestone closed

[neteler]

Ticket retargeted after milestone closed

[neteler]

Ticket retargeted after milestone closed