r.import: Fix the -e flag for estimating the resolution without importing #4947

KandelN · 2025-01-14T12:40:41Z

Problem Summary

When using r.import with the -e flag to estimate the resolution of a raster file without performing a full import, the following issues were observed:

The estimated resolution is not calculated or displayed if no reprojection is required.
A temporary raster map (rimport_tmp) remains in the mapset, which should be cleaned up.

Root Cause

The issue arises due to improper handling of cases where the raster input is already in the same projection as the current GRASS GIS session. There is no code for -e flag behavior when no reprojection is required. Additionally, the temporary raster map is not deleted.

Fixes

Resolution Estimation Logic:

Added a -e flag behavior when reprojection is not required. Applied the logic to ensure the resolution is calculated and displayed even when no reprojection is required.

Temporary Map Cleanup:

Created a new flag TMP_EST_FILE to hold the temporary file. Ensured that the temporary map (rimport_tmp) is deleted after the resolution estimation process.

github-actions · 2025-01-14T12:55:26Z

scripts/r.import/r.import.py

+    ):
+        gs.run_command(
+            "g.remove", type="raster", name=TMP_EST_FILE, flags="f", quiet=True
+        )

 def is_projection_matching(GDALdatasource):


[ruff] _{reported by reviewdog 🐶}

Suggested change

def is_projection_matching(GDALdatasource):

def is_projection_matching(GDALdatasource):

github-actions · 2025-01-14T12:55:26Z

scripts/r.import/r.import.py

+                    gs.message(_("Calculating estimted resolution..."))
+                    raster_info = gs.raster_info(output)
+                    if raster_info:
+                        estres = (raster_info["ewres"] + raster_info["nsres"])/2.0


[ruff] _{reported by reviewdog 🐶}

Suggested change

estres = (raster_info["ewres"] + raster_info["nsres"])/2.0

estres = (raster_info["ewres"] + raster_info["nsres"]) / 2.0

To be consistent with the original code: estres = math.sqrt(raster_info["nsres"] * raster_info["ewres"])?

petrasovaa · 2025-01-17T02:55:26Z

I am not entirely sure this is needed. The resolution needs to be estimated because of the reprojection, when you don't reproject, the resolution is not 'estimated'. But I agree, that having consistent behavior of the e flag would make sense (for automated workflows for example). But in that case, wouldn't using r.external work better, so that you avoid the import, which can take a long time. With r.external, the raster is only linked and I believe you can get correct resolution with r.info.

petrasovaa · 2025-01-17T02:57:00Z

scripts/r.import/r.import.py

@@ -168,6 +178,7 @@ def main():
    title = options["title"]
    if flags["e"] and not output:
        output = "rimport_tmp"  # will be removed with the entire tmp location
+        TMP_EST_FILE = output


I think you need something unique, see gs.append_node_pid below.

HuidaeCho · 2025-01-17T14:08:40Z

scripts/r.import/r.import.py

+                    gs.message(_("Calculating estimted resolution..."))
+                    raster_info = gs.raster_info(output)
+                    if raster_info:
+                        estres = (raster_info["ewres"] + raster_info["nsres"])/2.0


To be consistent with the original code: estres = math.sqrt(raster_info["nsres"] * raster_info["ewres"])?

HuidaeCho · 2025-01-17T14:24:00Z

scripts/r.import/r.import.py

+                    return 0
+                except CalledModuleError:
+                    gs.fatal(_("Unable to import GDAL dataset <%s>") % GDALdatasource)
+


No need for another try except and move this if flags["e"] block with return 0 below the original try except

try: gs.run_command("r.in.gdal", **parameters) ... return 0 # <-- delete this line except ... if flags["e"]: ... return 0

HuidaeCho · 2025-01-17T14:24:30Z

scripts/r.import/r.import.py

@@ -206,6 +217,25 @@ def main():
                _("Input <%s> successfully imported without reprojection")
                % GDALdatasource
            )
+            if flags["e"]:
+                try:
+                    gs.message(_("Calculating estimted resolution..."))


Suggested change

gs.message(_("Calculating estimted resolution..."))

gs.message(_("Calculating estimated resolution..."))

HuidaeCho · 2025-01-17T14:25:18Z

scripts/r.import/r.import.py

@@ -168,6 +178,7 @@ def main():
    title = options["title"]
    if flags["e"] and not output:
        output = "rimport_tmp"  # will be removed with the entire tmp location


As @petrasovaa suggested,

Suggested change

output = "rimport_tmp" # will be removed with the entire tmp location

output = gs.append_node_pid("rimport_tmp") # will be removed with the entire tmp location

HuidaeCho · 2025-01-17T14:28:32Z

scripts/r.import/r.import.py

+                    gs.message(_("Calculating estimted resolution..."))
+                    raster_info = gs.raster_info(output)
+                    if raster_info:
+                        estres = (raster_info["ewres"] + raster_info["nsres"])/2.0


To be consistent with the original code,

Suggested change

estres = (raster_info["ewres"] + raster_info["nsres"])/2.0

estres = math.sqrt(raster_info["nsres"] * raster_info["ewres"])

HuidaeCho · 2025-01-17T14:31:29Z

I am not entirely sure this is needed. The resolution needs to be estimated because of the reprojection, when you don't reproject, the resolution is not 'estimated'. But I agree, that having consistent behavior of the e flag would make sense (for automated workflows for example). But in that case, wouldn't using r.external work better, so that you avoid the import, which can take a long time. With r.external, the raster is only linked and I believe you can get correct resolution with r.info.

I think this flag needs to be fixed for consistency, but you're right about r.external. I think this PR can be improved for -e without reprojection using r.external?

@KandelN, you want to look into this possibility?

KandelN added 2 commits January 13, 2025 17:10

Add case for estimating resolution if projection matches

0e1d8e0

Modify tmp file name

0bffc66

github-actions bot added raster Related to raster data processing Python Related code is in Python module labels Jan 14, 2025

Merge branch 'main' into rimport-fix

6559c2d

echoix linked an issue Jan 14, 2025 that may be closed by this pull request

[Bug] r.import with e flag behaves unexpected, when no reprojection required #3252

Open

github-actions bot reviewed Jan 14, 2025

View reviewed changes

HuidaeCho self-assigned this Jan 14, 2025

HuidaeCho changed the title ~~Fix for Issue #3252 - r.import with -e Flag Behaving Unexpectedly When No Reprojection is Required~~ r.import: Fix the -e flag for estimating the resolution without importing Jan 14, 2025

HuidaeCho mentioned this pull request Jan 16, 2025

Develop a GRASS Conda package that will be useable across multiple OS platforms #3478

Open

petrasovaa reviewed Jan 17, 2025

View reviewed changes

HuidaeCho requested changes Jan 17, 2025

View reviewed changes

HuidaeCho reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r.import: Fix the -e flag for estimating the resolution without importing #4947

r.import: Fix the -e flag for estimating the resolution without importing #4947

KandelN commented Jan 14, 2025

github-actions bot Jan 14, 2025

github-actions bot Jan 14, 2025

HuidaeCho Jan 17, 2025

petrasovaa commented Jan 17, 2025

petrasovaa Jan 17, 2025

HuidaeCho Jan 17, 2025

HuidaeCho Jan 17, 2025 •

edited

Loading

HuidaeCho Jan 17, 2025

HuidaeCho Jan 17, 2025

HuidaeCho Jan 17, 2025

HuidaeCho commented Jan 17, 2025 •

edited

Loading

	def is_projection_matching(GDALdatasource):

	def is_projection_matching(GDALdatasource):

	estres = (raster_info["ewres"] + raster_info["nsres"])/2.0
	estres = (raster_info["ewres"] + raster_info["nsres"]) / 2.0

	gs.message(_("Calculating estimted resolution..."))
	gs.message(_("Calculating estimated resolution..."))

	output = "rimport_tmp" # will be removed with the entire tmp location
	output = gs.append_node_pid("rimport_tmp") # will be removed with the entire tmp location

	estres = (raster_info["ewres"] + raster_info["nsres"])/2.0
	estres = math.sqrt(raster_info["nsres"] * raster_info["ewres"])

r.import: Fix the -e flag for estimating the resolution without importing #4947

Are you sure you want to change the base?

r.import: Fix the -e flag for estimating the resolution without importing #4947

Conversation

KandelN commented Jan 14, 2025

Problem Summary

Root Cause

Fixes

Resolution Estimation Logic:

Temporary Map Cleanup:

github-actions bot Jan 14, 2025

Choose a reason for hiding this comment

github-actions bot Jan 14, 2025

Choose a reason for hiding this comment

HuidaeCho Jan 17, 2025

Choose a reason for hiding this comment

petrasovaa commented Jan 17, 2025

petrasovaa Jan 17, 2025

Choose a reason for hiding this comment

HuidaeCho Jan 17, 2025

Choose a reason for hiding this comment

HuidaeCho Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

HuidaeCho Jan 17, 2025

Choose a reason for hiding this comment

HuidaeCho Jan 17, 2025

Choose a reason for hiding this comment

HuidaeCho Jan 17, 2025

Choose a reason for hiding this comment

HuidaeCho commented Jan 17, 2025 • edited Loading

HuidaeCho Jan 17, 2025 •

edited

Loading

HuidaeCho commented Jan 17, 2025 •

edited

Loading