temporal: t.rast.aggregate: add -e flag to extend existing STRDS by Sourish-spc · Pull Request #7223 · OSGeo/grass

Sourish-spc · 2026-03-27T07:32:30Z

Description

This PR adds support for extending an existing Space Time Raster Dataset (STRDS) in t.rast.aggregate by introducing a new -e flag, following the same pattern implemented in #3798 for t.rast.neighbors.

Changes

Added -e flag (Extend existing space time raster dataset) to module interface
Modified STRDS creation logic to open existing STRDS when -e flag is set instead of always creating a new one
Added history update via update_command_string() when extending existing STRDS
Added test test_extend_existing_strds to existing test suite

Usage

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

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

ninsbl

Please also install pre-commit in your local working directory to avoid CI runs for formalities...

ninsbl · 2026-03-27T08:51:44Z

+        gs.fatal(
+            _(
+                "Space time raster dataset <%s> does not exist. "
+                "Cannot extend a non-existing dataset."
+            )
+            % output
+        )


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.

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

wenzeslaus · 2026-03-28T04:07:07Z

+        # 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)


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.

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.

wenzeslaus · 2026-03-28T04:11:36Z

+        if output_exists:
+            output_strds.update_command_string(dbif=dbif)


Does this make sense when overwriting?

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

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.

wenzeslaus · 2026-03-28T04:12:08Z


 if __name__ == "__main__":
    options, flags = gs.parser()
+


Please, review your code, including the PR changes.

Thank you for the review. The extra blank line has been removed in the latest commit.

…ctions for extend support

ninsbl

Please keep in mind that the code in open_stds pre-dates Python 3 and has not been re-factored after since.
We should not do re-factoring as part of this PR, but don`t repeat outdated pattern in stuff you add....

ninsbl · 2026-04-03T09:34:22Z

+    dbif, connection_state_changed = init_dbif(dbif)
+    msgr = get_tgis_message_interface()
+
+    mapset = get_current_mapset()
+    id = name + "@" + mapset if name.find("@") < 0 else name
+
+    if type in {"strds", "rast", "raster"}:
+        sp = dataset_factory("strds", id)
+    elif type in {"str3ds", "raster3d", "rast3d", "raster_3d"}:
+        sp = dataset_factory("str3ds", id)
+    elif type in {"stvds", "vect", "vector"}:
+        sp = dataset_factory("stvds", id)
+    else:
+        msgr.fatal(_("Unknown type: %s") % (type))
+    if extend and sp.is_in_db(dbif):
+        # extend-if-exists: load and return existing dataset
+        sp.select(dbif)
+        if connection_state_changed:
+            dbif.close()
+        return sp


You duplicate quite a bit of code... You may consider adding a helper function similar to e.g. (untested, so not suitable for copy and paste!):

def get_stds(stds_id: str, stds_type: str) -> SpaceTimeDataset: """Return an initialized SpaceTimeDataset (STDS).""" msgr = get_tgis_message_interface() supported_stds_types = { "strds": "strds", "rast": "strds", "raster": "strds", "str3ds": "str3ds", "raster3d": "str3ds", "rast3d": "str3ds", "raster_3d": "str3ds", "stvds": "stvds", "vect": "stvds", "vector": "stvds", } if stds_type not in supported_stds_types: msgr.fatal(_("Unknown type: %s") % (type)) return dataset_factory(supported_stds_types[stds_type], stds_id)]

Added _get_stds helper function to avoid repeating the type mapping block, and ensure_id helper to avoid repeating the id construction pattern. Kept them private ( prefix) since they're only used internally in open_stds.py happy to make them public if needed.

…hon style

…TimeDataset

ninsbl

Now we are getting somewhere. Almost done. I just had some hopefully final, minor comments.
Once those are addressed, this can be approved as far as I am concerned. I would appreciate a second opinion, especially on having the new helper function public or private...

ninsbl · 2026-04-05T10:01:53Z

+    dbif, connection_state_changed = init_dbif(dbif)
+    output_exists = sp.is_in_db(dbif)
+
+    if output_exists:


See the comment above about collapsible-if...

Fixed: collapsed nested if statements into single conditions.

ninsbl · 2026-04-05T10:03:18Z

+    """
+    msgr = get_tgis_message_interface()
+
+    if "@" in name:


You can skip this check if you _ensure-id before.
If ensure_id is a public function and used in t.rast.aggregate, the new functions you add here could expect an id as input and those checks are not needed at all. Yet, they are insignificant and may be safer to double-check. No strong opinion from my side on this...

Removed the redundant mapset check

ninsbl · 2026-04-05T10:28:29Z

+        )
+
+        # Check original maps are still present (STRDS was not overwritten)
+        for original_map in original_maps.strip().split(os.linesep):


Maybe use all() or a set() based check and only one assertion...

Replaced the loop with a single set.issubset() assertion.

ninsbl · 2026-04-05T10:32:37Z

+            self.assertIn(original_map, extended_maps)
+
+        # Check total map count increased (original 3 + extended 3 = 6)
+        info = SimpleModule("t.info", flags="g", input="B")


You get the number of maps from t.rast.list. No need to run t.info in addition for this check. If you want to check that the STRDS history is updated properly (command added to comments) you need to.info...

Removed the t.info call, map count is now derived directly from t.rast.list output using len().

Sourish-spc · 2026-04-06T12:34:55Z

Note: test_aggregation_relative.py was already failing before this PR's changes, verified by running the tests on the original code via git stash. This appears to be a pre-existing issue unrelated to the -e flag changes."

…check, simplify test assertions

ninsbl

Two minor and hopefully final comments from my side. Looks otherwise good. I would appreciate a second opinion though, esp. regarding the library parts...

Co-authored-by: Stefan Blumentrath <stefan.blumentrath@gmx.de>

ninsbl

Thanks for addressing my comments. @wenzeslaus What do you think? Ready to merge?

Sourish-spc · 2026-05-16T03:53:25Z

Any updates on this? Happy to address any further feedback.

ninsbl

Looks good to me. @petrasovaa could you double-check the test and library code? Would be good to get this merged, so the functions can be reused...

petrasovaa · 2026-05-21T03:12:46Z

+    id = _ensure_id(name)
+    sp = _get_stds(id, type)
+
+    if extend and sp.is_in_db(dbif):


Should this be using check_new_stds somewhere?

Otherwise check you are writing/modifying data in current mapset is missing.

Addressed your review comments:

Added mapset ownership check when extending
Added temporal type compatibility check early in check_open_output_stds() before processing starts
Added fatal error when -e is used but output STRDS does not exist
Added two new test cases covering both edge cases

petrasovaa · 2026-05-21T03:33:28Z

I think the tests don't quite cover, e.g. -e and stds doesn't exist, extending with stds with incompatible temporal type...

- Add mapset ownership check in open_output_stds() when extending - Add temporal type compatibility check in check_open_output_stds() before processing starts to avoid unnecessary computation - Add temporaltype parameter to check_open_output_stds() - Fatal error when -e flag used but output STRDS does not exist - Add tests for nonexistent STRDS and incompatible temporal type cases

github-actions Bot added temporal Related to temporal data processing Python Related code is in Python libraries module tests Related to Test Suite labels Mar 27, 2026

Sourish-spc force-pushed the temporal/t.rast.aggregate-extend-mode branch from 7ee4adc to 9b2110e Compare March 27, 2026 07:45

github-actions Bot reviewed Mar 27, 2026

View reviewed changes

Comment thread temporal/t.rast.aggregate/t.rast.aggregate.py Resolved

Comment thread temporal/t.rast.aggregate/testsuite/test_aggregation_absolute.py Resolved

ninsbl requested changes Mar 27, 2026

View reviewed changes

Sourish-spc added 9 commits March 27, 2026 12:51

fix: resolve ruff linting issues

9b2110e

fix: resolve remaining ruff linting issues

a39bdee

Merge branch 'main' into temporal/t.rast.aggregate-extend-mode

3f76909

fix: resolve extra line issue

e0de432

fix: add trailing newline at end of files

6ec7ac4

fix: resolve extra line issue

ca3f87e

fix: add trailing newline at end of files

d878245

fix: use warning instead of fatal when extending non-existing STRDS

1ca0f6e

wenzeslaus requested changes Mar 28, 2026

View reviewed changes

Sourish-spc added 8 commits March 29, 2026 10:41

temporal: refactor extend logic into check_new_stds library function

d73d2a5

Merge branch 'main' into temporal/t.rast.aggregate-extend-mode

36700a4

temporal: strengthen test for -e flag to verify extend behavior

75d75b0

temporal: remove extra blank line in __main__ block

0d65b33

temporal: fix metadata overwrite bug on extend and resolve ruff RUF052

58f6d4b

temporal: add check_open_output_stds and open_output_stds library fun…

7e3d570

…ctions for extend support

Merge branch 'main' into temporal/t.rast.aggregate-extend-mode

08a838c

Merge branch 'main' into temporal/t.rast.aggregate-extend-mode

5362536

ninsbl requested changes Apr 3, 2026

View reviewed changes

Sourish-spc added 2 commits April 4, 2026 15:36

temporal: refactor extend logic using helper functions and modern Pyt…

201bbd7

…hon style

Merge branch 'main' into temporal/t.rast.aggregate-extend-mode

7260037

Sourish-spc added 3 commits April 4, 2026 16:13

temporal: add return type annotation to _get_stds using AbstractSpace…

215f13b

…TimeDataset

temporal: fix docstring to use AbstractSpaceTimeDataset in _get_stds

29414ef

temporal: add type annotations for dbif parameter in new functions

c731f6f

ninsbl requested changes Apr 5, 2026

View reviewed changes

temporal: address review comments - collapsible-if, remove redundant …

eb21643

…check, simplify test assertions

ninsbl requested changes Apr 12, 2026

View reviewed changes

Comment thread temporal/t.rast.aggregate/t.rast.aggregate.py Resolved

Comment thread temporal/t.rast.aggregate/testsuite/test_aggregation_absolute.py Resolved

Update temporal/t.rast.aggregate/testsuite/test_aggregation_absolute.py

ea4d6f8

Co-authored-by: Stefan Blumentrath <stefan.blumentrath@gmx.de>

ninsbl approved these changes Apr 14, 2026

View reviewed changes

ninsbl previously approved these changes May 17, 2026

View reviewed changes

petrasovaa reviewed May 21, 2026

View reviewed changes

Sourish-spc dismissed ninsbl’s stale review via c494a72 May 27, 2026 03:46

		if output_exists:
		output_strds.update_command_string(dbif=dbif)

Conversation

Sourish-spc commented Mar 27, 2026

Description

Changes

Usage

Related

Uh oh!

Uh oh!

Uh oh!

ninsbl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ninsbl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sourish-spc Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ninsbl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sourish-spc commented Apr 6, 2026

Uh oh!

ninsbl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ninsbl left a comment

Choose a reason for hiding this comment

Uh oh!

Sourish-spc commented May 16, 2026

Uh oh!

Sourish-spc Apr 5, 2026 •

edited

Loading