Skip to content

Fix inverted resolution check in _line_to_pnts (under-resolved line warning)#1303

Open
martinguthrie93 wants to merge 2 commits into
CLIMADA-project:developfrom
martinguthrie93:fix/line-resolution-warning
Open

Fix inverted resolution check in _line_to_pnts (under-resolved line warning)#1303
martinguthrie93 wants to merge 2 commits into
CLIMADA-project:developfrom
martinguthrie93:fix/line-resolution-warning

Conversation

@martinguthrie93

Copy link
Copy Markdown

Changes proposed in this PR:

  • Fix the inverted resolution check in climada.util.lines_polys_handler._line_to_pnts. The mask used to count under-resolved lines selected lines longer than 10 * resolution (line_lengths > 10 * res), while the inline comment and the log message both describe lines shorter than 10 * resolution. The comparison is now < 10 * res, so the warning is raised for the lines it is meant to flag.
  • Correct the wording and typos of the warning message (disaggregatedisaggregated, ReaggregatintReaggregating, overestimattionoverestimation, chosingchoosing).
  • Update test_resolution_warning, which previously asserted the buggy behavior (it expected 2 flagged lines for input lengths [2, 12, 20] with res = 1, even though lengths 12 and 20 are longer than 10 * res = 10). The expected count is now 1, corresponding to the single genuinely under-resolved line (length 2).

Why this matters

When a line exposure (road, pipeline, transmission line, river reach, …) is disaggregated into points, the number of points is round(length / res). A line that is short relative to res collapses to very few points — in the limit, a single representative point — and re-aggregating an impact computed on too few points typically overestimates that feature's impact. The warning exists to alert users to this situation.

Because the comparison was inverted:

  • users with genuinely under-resolved lines received no warning (silent overestimation), and
  • users with well-resolved long lines received a warning whose text contradicted itself.

Only the warning path is affected; the returned geometry is unchanged.

Verification

The existing test test_resolution_warning had encoded the contradiction (asserting "2 lines with a length < 10*resolution" for two lines that are in fact longer than 10 * res). I reproduced the mask logic in isolation to confirm the corrected behavior:

import numpy as np
line_lengths = np.array([2.0, 12.0, 20.0]); res = 1  # 10 * res == 10
len(line_lengths[line_lengths > 10 * res])  # old: 2 (the LONG lines 12, 20)
len(line_lengths[line_lengths < 10 * res])  # new: 1 (the short line 2)

Note: the full CLIMADA test suite was not run in my local environment (CLIMADA and its geospatial dependencies were not installed there); the change is confined to the warning branch and its dedicated unit test, which I verified matches exactly.

This PR fixes #1302

PR Author Checklist

  • Read the Contribution Guide
  • Correct target branch selected (if unsure, select develop)
  • Descriptive pull request title added
  • Source branch up-to-date with target branch
  • Documentation updated
  • Tests updated
  • Tests passing (verified the affected unit test's logic in isolation; the full suite was not run locally — see note above)
  • No new linter issues (black reports both changed files unchanged)
  • Changelog updated

The warning meant to flag under-resolved line exposures in
`climada.util.lines_polys_handler._line_to_pnts` used an inverted
comparison: it counted lines *longer* than `10 * resolution` while its
comment and message describe lines *shorter* than that. As a result,
genuinely under-resolved lines (which collapse to too few points and
overestimate the re-aggregated impact) were skipped silently, while
well-resolved long lines produced a spurious, self-contradicting warning.

Flip the comparison to `< 10 * res`, correct the message wording and
typos, and update `test_resolution_warning` accordingly.

Fixes CLIMADA-project#1302

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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.

1 participant