Skip to content

r.local.relief: Fix TypeError in bspline mode, pass spline_step, fix color table - #1891

Open
bhuvan-somisetty wants to merge 7 commits into
OSGeo:grass8from
bhuvan-somisetty:fix-r-local-relief-1890
Open

bhuvan-somisetty wants to merge 7 commits into
OSGeo:grass8from
bhuvan-somisetty:fix-r-local-relief-1890

Conversation

@bhuvan-somisetty

@bhuvan-somisetty bhuvan-somisetty commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1890

Summary of Changes

This PR fixes issues in r.local.relief in bicubic spline interpolation mode (-v) and when generating shaded relief output:

  1. Fix TypeError: 'dict_keys' object is not subscriptable and small-unit spline_step estimation:

    • The spline step was taken by indexing dict.keys() from gs.parse_command("v.surf.bspline", flags="e", ...), which fails in Python 3. Parsing that output is also fragile because v.surf.bspline -e prints a translatable message, not key=value pairs.
    • The spline step is now computed directly from gs.vector_info() of the contour points as sqrt(extent_area / n_points) * 2 (falling back to the computational region area if the contour extent area is zero), matching v.surf.bspline -e (P_estimate_splinestep). The module fails with a clear message if there are no contour points.
    • Removed round() from the spline_step calculation so small-unit projects (such as latitude-longitude in degrees) do not round spline_step down to 0.
  2. Pass computed spline_step to v.surf.bspline:

    • The spline step was computed and reported with gs.info, but never passed to v.surf.bspline. Added ew_step=spline_step and ns_step=spline_step.
  3. Fix color table name in shaded output:

    • Changed the default color table from "difference" to "differences", the valid GRASS color table name.
  4. Add tests:

    • Added testsuite/test_r_local_relief.py covering the default r.fillnulls mode, -v bspline mode, and shaded output.
    • Added tests/r_local_relief_latlon_test.py verifying -v bspline mode in a latitude-longitude project.

Signed-off-by: bhuvan-somisetty somisettybhuvan5@gmail.com

…step, fix color table name

In the bspline interpolation branch (-v flag):
- Replace gs.parse_command with gs.read_command to parse v.surf.bspline -e
  output; dict.keys()[0] raises TypeError in Python 3 since dict_keys is
  not subscriptable.
- Pass the computed spline_step as ew_step and ns_step to v.surf.bspline;
  previously spline_step was computed but silently ignored.
- Fix fallback color table name from "difference" to "differences" in the
  shaded output path.
- Add gunittest test suite covering default, bspline, and shaded output modes.

Fixes OSGeo#1890

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
v.surf.bspline requires raster_output even when run with -e (estimation)
flag. Without it the module's parser returns exit code 1.

Additionally, v.surf.bspline -e reports the estimated mean distance via
G_message() which writes to stderr, not stdout. Replace gs.read_command
(stdout-only) with gs.start_command capturing both stdout and stderr so
the distance value is reliably parsed regardless of where it appears.

Import subprocess at the top of the file to support subprocess.PIPE.

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
gs.start_command communicate() returns str objects (text mode) in the
GRASS Python scripting environment, not bytes. Calling .decode() on a
str raises AttributeError. Handle both str and bytes to be safe across
GRASS versions.

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
combined_output = out_text + err_text
spline_step = None
for line in combined_output.splitlines():
if ":" in line and "distance" in line.lower():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

G_message are user messages, so they may be translated. This then fails. Other options?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Replaced it with v.info -t and gs.region() to compute the mean distance directly (sqrt(area / points)), so it no longer relies on parsing localized messages.

…gion

G_message output from v.surf.bspline -e may be translated, making
parsing unreliable in non-English locales. Instead, replicate the same
estimation formula (mean_dist = sqrt(area / n_points)) using v.info -t
(machine-readable key=value output) and gs.region(), which are locale-
independent. Also removes the subprocess import which is no longer needed.

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
gs.raster_info() returns dictionary values as strings (e.g.,
info["cells"] is a str). Comparing info["cells"] > 0 directly raised
TypeError: '>' not supported between instances of 'str' and 'int'.
Cast cells to int and min/max to float before assertions.

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>

@wenzeslaus wenzeslaus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, keep the PR desc in sync with the code.

Comment on lines +289 to +292
else:
spline_step = 10
else:
spline_step = 10

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is low quality code

Comment on lines +280 to +283
except Exception:
region = None
vinfo = {}
if region:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overly complicated and a general exception. See gs.vector_info_topo

Compute the mean point distance from the point count and extent of the
contour points, as v.surf.bspline -e does, instead of region area with a
broad exception handler and hardcoded fallbacks. Fail with a clear
message when no contour points exist.

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
@bhuvan-somisetty

Copy link
Copy Markdown
Contributor Author

thanks for the review. cleaned up the spline step part, it now just uses gs.vector_info on the contour points and computes sqrt(extent area / points) * 2, same as what v.surf.bspline -e does internally. no more broad except or hardcoded fallbacks, it fails with a clear message if there are no contour points. also updated the PR description to match the current code.

if not info["points"]:
gs.fatal(_("No points found on the zero contours"))
area = (info["east"] - info["west"]) * (info["north"] - info["south"])
spline_step = round(math.sqrt(area / info["points"])) * 2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With round() the spline step is 0 when the mean point distance is below 0.5 map units, e.g., in a latitude-longitude project, and v.surf.bspline then never finishes. Please add a test for this, such as the one below (pytest, tests/r_local_relief_latlon_test.py), and make it pass.

"""Test r.local.relief in a latitude-longitude project"""

import os

import pytest

import grass.script as gs
from grass.tools import Tools


@pytest.fixture
def latlon_session(tmp_path):
    """Latitude-longitude project with a synthetic wavy elevation raster"""
    project = tmp_path / "project"
    gs.create_project(project, epsg="4326")
    with gs.setup.init(project, env=os.environ.copy()) as session:
        tools = Tools(session=session)
        tools.g_region(n=36, s=35.95, w=-79, e=-78.95, res="0:00:03")
        tools.r_mapcalc(
            expression=(
                "elevation = 100 + 20 * sin(row() * 12) * cos(col() * 9)"
                " + 3 * sin(row() * 47 + col() * 31)"
            )
        )
        yield session


def test_bspline_latlon(latlon_session):
    """Check that -v gives a full local relief raster when map units are degrees"""
    tools = Tools(session=latlon_session)
    tools.r_local_relief(
        input="elevation", output="local_relief", neighborhood_size=5, flags="v"
    )
    stats = tools.r_univar(map="local_relief", format="json")
    assert stats["null_cells"] == 0
    assert stats["min"] < 0 < stats["max"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, removed round() so small units like degrees don't collapse to 0 and added tests/r_local_relief_latlon_test.py. Updated the PR description as well.

Removing round() when computing spline_step allows floating-point spline
step values in latitude-longitude and other small-unit CRS projects where
mean point distance is below 0.5 map units (previously rounding to 0 and
causing v.surf.bspline to hang). Fall back to the computational region
area if contour point extent area is zero. Add a pytest test in a
latitude-longitude project.

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] r.local.relief: TypeError on dict_keys in bspline interpolation mode (-v) and unused spline_step

2 participants