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

temporal/t.rast.aggregate/t.rast.aggregate.py

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

ninsbl

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

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

temporal/t.rast.aggregate/t.rast.aggregate.py

+        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

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

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

temporal/t.rast.aggregate/t.rast.aggregate.py

+        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

temporal/t.rast.aggregate/t.rast.aggregate.py


 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.

temporal/t.rast.aggregate/t.rast.aggregate.py

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

temporal/t.rast.aggregate/t.rast.aggregate.py

python/grass/temporal/open_stds.py

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

python/grass/temporal/open_stds.py

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

python/grass/temporal/open_stds.py

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

python/grass/temporal/open_stds.py

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

python/grass/temporal/open_stds.py

+    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

python/grass/temporal/open_stds.py

+    """
+    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

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

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

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

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

ninsbl · 2026-04-12T21:14:56Z

temporal/t.rast.aggregate/t.rast.aggregate.py

+        # Refresh STRDS object to reflect newly registered maps
+        output_strds.select(dbif)


What is the reason to add this here? Was it a necessary change? And what does it accomplish?

Yes, it was necessary to fix a bug. Without it, after register_map_object_list adds the new maps into the existing STRDS, the in-memory object still holds the old state (3 maps). select(dbif) refreshes it from the database so metadata.update records the correct map count (6). Removing it causes the extend test to fail.

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

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

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

temporal/t.rast.aggregate/t.rast.aggregate.py Outdated Show resolved Hide resolved

temporal/t.rast.aggregate/testsuite/test_aggregation_absolute.py Outdated Show resolved Hide 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

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

ea4d6f8

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

		if output_exists:
		output_strds.update_command_string(dbif=dbif)

		# Refresh STRDS object to reflect newly registered maps
		output_strds.select(dbif)

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Sourish-spc Apr 5, 2026 •

edited

Loading