temporal: t.rast.aggregate: add -e flag to extend existing STRDS#7223
temporal: t.rast.aggregate: add -e flag to extend existing STRDS#7223Sourish-spc wants to merge 15 commits intoOSGeo:mainfrom
Conversation
In stds_import.py, the finally block used the deprecated 'gisdbase' option when calling g.mapset to switch back to the original location. This caused a deprecation warning on every t.rast.import run with the project parameter. Replaced 'gisdbase' with 'dbase' to match the current g.mapset API. Fixes OSGeo#4231
7ee4adc to
9b2110e
Compare
temporal/t.rast.aggregate/testsuite/test_aggregation_absolute.py
Outdated
Show resolved
Hide resolved
ninsbl
left a comment
There was a problem hiding this comment.
Please also install pre-commit in your local working directory to avoid CI runs for formalities...
| gs.fatal( | ||
| _( | ||
| "Space time raster dataset <%s> does not exist. " | ||
| "Cannot extend a non-existing dataset." | ||
| ) | ||
| % output | ||
| ) |
There was a problem hiding this comment.
Personally, I do not think a fatal error should be thrown here. I would say, a warning at best. In any case, this part should be consistent across all temporal tools that receive the e-flag.
There was a problem hiding this comment.
Updated: changed fatal error to warning and fall back to creating new STRDS when dataset doesn't exist.
Adds support for extending an existing Space Time Raster Dataset by running t.rast.aggregate with the -e flag. This allows recurring workflows to append new aggregated results to an existing STRDS without deleting previous data. See OSGeo#3427
| # Verify STRDS was extended | ||
| lister = SimpleModule("t.rast.list", input="B", columns="name", flags="u") | ||
| self.runModule(lister) | ||
| self.assertIn("b_ext", lister.outputs.stdout) |
There was a problem hiding this comment.
This only checks that some map with "b_ext" in the name exists. It doesn't verify the original maps are still present, that the STRDS wasn't just overwritten, and/or the total map count.
There was a problem hiding this comment.
Thank you for the review. The test has been strengthened to also verify that original maps are still present and the total map count increased after extending.
| if output_exists: | ||
| output_strds.update_command_string(dbif=dbif) |
There was a problem hiding this comment.
Does this make sense when overwriting?
There was a problem hiding this comment.
Good point, iI did not really think about it. It may make sense in any case. I did not check the behavior. Updates of the STDS should be written to the history I`ld say, which is what update_command_string does. The command should not be duplicated in the history though. That should be checked...
There was a problem hiding this comment.
Updated the condition to if not output_exists or extend to avoid duplicating the command string in history when overwriting, while still updating it correctly when extending.
|
|
||
| if __name__ == "__main__": | ||
| options, flags = gs.parser() | ||
|
|
There was a problem hiding this comment.
Please, review your code, including the PR changes.
There was a problem hiding this comment.
Thank you for the review. The extra blank line has been removed in the latest commit.
| if not output_exists or (overwrite and not extend): | ||
| temporal_type, semantic_type, title, description = sp.get_initial_values() | ||
| output_strds = tgis.open_new_stds( | ||
| output, | ||
| "strds", | ||
| temporal_type, | ||
| title, | ||
| description, | ||
| semantic_type, | ||
| dbif, | ||
| overwrite, | ||
| ) | ||
| elif output_exists and extend: | ||
| output_strds = tgis.open_old_stds(output, "strds", dbif) |
There was a problem hiding this comment.
What happens when "else"? Or is there a condition which I don't see. Perhaps worth commenting on if that's the case.
There was a problem hiding this comment.
I noticed that in the.rast.neighbors too, where this is likely copied from. So It should be addressed there in a different PR too. Given that the possibility to extend existing STDS would be useful in multiple temporal tools, maybe a extend_if_exists (or something like that) library function would make sense, that returns either a new or exiting STDS!? @wenzeslaus what do you think?
There was a problem hiding this comment.
Maybe enhance and use
grass/python/grass/temporal/open_stds.py
Line 91 in 65a1515
There was a problem hiding this comment.
Refactored the extend logic into check_new_stds in open_stds.py as a reusable library function with an extend parameter. The else case is now handled there: if -e is set and STRDS does not exist, it silently creates a new one (extend-if-exists behavior).
There was a problem hiding this comment.
Hi, an sorry for possibly putting you on a slightly wrong track with enhancing check_new_stds . That function should probably stay as is.
What in general is needed is 1) a function that checks if output STDS can be opened as requested (new or existing for update, e.g. check_open_output_stds). That should be minimal (not creating or opening) and run before main processing starts. And 2), after main processing succeeded, the output STDS should be opened or created so produced maps can be registered (e.g. open_output_stds).
| if extend and not output_exists: | ||
| gs.warning( | ||
| _( | ||
| "Space time raster dataset <%s> does not exist. " | ||
| "Cannot extend a non-existing dataset." | ||
| ) | ||
| % output | ||
| ) | ||
| extend = False # fall back to creating new STRDS |
There was a problem hiding this comment.
The warning does not make sense to me. Either -e is "extend if already exist" (no error or warning needed) or "extend existing" (so error on non-existence). The first option is sort of append mode; we have that elsewhere.
There was a problem hiding this comment.
Agreed. That said, the confusion is my fault as I have not been explicit.
Personally, I would prefer "extend-if-exists" behavior, as it is more graceful, but I have no strong opinion here...
There was a problem hiding this comment.
Removed the warning and fallback. Now using extend-if-exists behavior: if the STRDS doesn't exist, it just creates a new one silently.
Description
This PR adds support for extending an existing Space Time Raster Dataset (STRDS) in
t.rast.aggregateby introducing a new-eflag, following the same pattern implemented in #3798 fort.rast.neighbors.Changes
-eflag (Extend existing space time raster dataset) to module interface-eflag is set instead of always creating a new oneupdate_command_string()when extending existing STRDStest_extend_existing_strdsto existing test suiteUsage
First run — creates new STRDS:
t.rast.aggregate input=daily_rain output=monthly_rain granularity="1 month" method=average basename=result
Second run — extends existing STRDS:
t.rast.aggregate input=daily_rain output=monthly_rain granularity="1 month" method=average basename=result_new -e
Related
Closes part of #3427
See also #3798