Skip to content

Stop dft_rast_classify() modifying the caller's raster (#89) - #93

Merged
NewGraphEnvironment merged 7 commits into
mainfrom
89-dft-rast-classify-mutates-the-caller-s-r
Sep 30, 2026
Merged

NewGraphEnvironment merged 7 commits into
mainfrom
89-dft-rast-classify-mutates-the-caller-s-r

Conversation

@NewGraphEnvironment

Copy link
Copy Markdown
Owner

Summary

  • dft_rast_classify() no longer modifies the caller's raster. It called the in-place terra::set.cats() on the object it was given. Now coltab<- runs first, which deep-copies, and the levels are set on that copy. The output is identical() to 0.19.0 (cats() and coltab()), and no extra copy is made.
  • A new test covers file-backed, in-memory and list-element inputs. It goes red on the old order with 6 failures. The workaround comment in test-dft_accuracy_sample.R is removed.
  • Release v0.19.1: NEWS, and a CLAUDE.md note on measuring peak RSS for short calls.

Related Issues

Scale check (BULK, per CLAUDE.md)

BULK classified_2017.tif (169,248,352 cells), terra 1.9.50. Kernel max RSS from /usr/bin/time -l, two reps each:

variant input max RSS (GiB) caller untouched
0.19.0 in memory 4.21 no
this PR in memory 4.21 yes
deepcopy() first (the issue's proposal) in memory 5.46 yes
0.19.0 / this PR file-backed 1.05 / 1.05 no / yes

deepcopy() would add 1.25 GiB per year classified in memory, so it was not used. The classify call itself takes 0.8–0.9 s on both main and this branch. The first attempt used a 2 s ps sampler, which cannot catch a sub-second peak: it read 0.26 GiB. I first blamed sampling the wrong PID, and retracted that after checking. The archive README records the full chain.

Test plan

  • devtools::test(): [ FAIL 0 | WARN 0 | SKIP 15 | PASS 1318 ]
  • New test red on the old order (6 failures), green on the fix
  • /code-check: three rounds, all clean. Reviewers confirmed that coltab<- copies at the terra 1.8-10 floor, that old and new orders give identical output on nine input shapes, and that no caller relied on the mutation.
  • Changed lines lint clean

🤖 Generated with Claude Code

https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN

NewGraphEnvironment and others added 7 commits September 29, 2026 17:14
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
set.cats() works in place and was called on the caller's own SpatRaster, so
classifying turned the original into a factor named class_name. Set the
colour table first: `coltab<-` deep-copies before it sets, so set.cats() now
only reaches that copy. Output is identical to the old order and no second
copy is made, as an explicit deepcopy() would.

A new test checks file-backed, in-memory and list-element inputs. It is red
on the old order with 6 failures. Removes the test-dft_accuracy_sample.R
comment that worked around the bug. Code-check: three rounds, all clean.

Fixes #89

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
@NewGraphEnvironment
NewGraphEnvironment merged commit 5a55ee4 into main Sep 30, 2026
1 check passed
@NewGraphEnvironment
NewGraphEnvironment deleted the 89-dft-rast-classify-mutates-the-caller-s-r branch September 30, 2026 00:31
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.

dft_rast_classify() mutates the caller's raster in place (set.cats on the input)

1 participant