Feature: CLUMPY style allocation#30
Merged
Merged
Conversation
…architecture Agent-Logs-Url: https://github.com/ethzplus/evoland-plus/sessions/39b6065d-0375-4ce9-ae25-40e65e2e68dd Co-authored-by: mmyrte <24587121+mmyrte@users.noreply.github.com>
…lock Agent-Logs-Url: https://github.com/ethzplus/evoland-plus/sessions/39b6065d-0375-4ce9-ae25-40e65e2e68dd Co-authored-by: mmyrte <24587121+mmyrte@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
mmyrte
May 12, 2026 13:49
View session
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
air
[air] reported by reviewdog 🐶
evoland-plus/inst/tinytest/test_alloc_params_t.R
Lines 79 to 80 in 2cfdcba
[air] reported by reviewdog 🐶
[air] reported by reviewdog 🐶
evoland-plus/inst/tinytest/test_trans_pot_t.R
Lines 11 to 12 in 2cfdcba
[air] reported by reviewdog 🐶
evoland-plus/inst/tinytest/test_trans_pot_t.R
Lines 37 to 38 in 2cfdcba
[air] reported by reviewdog 🐶
[air] reported by reviewdog 🐶
evoland-plus/inst/tinytest/test_trans_pot_t.R
Lines 43 to 44 in 2cfdcba
[air] reported by reviewdog 🐶
evoland-plus/inst/tinytest/test_trans_pot_t.R
Lines 48 to 49 in 2cfdcba
[air] reported by reviewdog 🐶
evoland-plus/inst/tinytest/test_trans_pot_t.R
Lines 81 to 82 in 2cfdcba
Copilot
AI
changed the title
feat: CLUMPY allocation backend + raw/adjusted transition potential architecture
Fix R CMD check lazy-load failure from duplicate R6 method registration
Jun 16, 2026
mmyrte
changed the title
mmyrte
force-pushed
the
copilot/discuss-transition-probability-estimation
branch
from
6a135de to
a672f32
Compare
…esis Adds dev/pivot-mechanism-verification.md documenting: - GART/MuST pivot test is a faithful inverse-CDF translation - the allocator implements the simplified single-pass strategy, not the bias-free uPAM (no per-patch P(v|u) update, no merge rollback) - the 1/E(sigma) pivot rarefaction (thesis Fig. 3.2) is missing - determinism behaviour under 0/1 transition potentials Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189ymK6BeCwBC8iZ45CGuVb
…hape metric
Move the whole CLUMPY pivot-selection + patch-growth routine into C++
(allocate_clumpy_cpp) so the hot loop no longer crosses into R per patch.
- Add selectable allocation methods:
* uSAM (single GART pass, quantity of change in expectation)
* uPAM (iterative GART with a per-transition pixel quota and sampling
without replacement; batch_size dials speed vs fidelity, 1 = strict
one-pivot-per-draw). Affordable here because evoland's fixed-model
potentials are pool-independent, so rho(z|u) is never re-estimated.
- Apply the 1/E(sigma) pivot rarefaction before GART (thesis Fig. 3.2) so the
allocated quantity of change matches the target rate; rate confirmed to be a
quantity-of-change rate via get_obs_trans_rates.
- Clamp negative/NaN potentials inside GART (matches reference clumpy).
- Move gart/sample_lognorm_area/raster_neighbors from R into C++
(gart_cpp, sample_lognorm_area_cpp, raster_neighbors_cpp).
- Unify the duplicate patch-shape metric (patch_eccentricity vs
calculate_elongation) into clumpy::elongation_from_raw_moments
(src/clumpy_geometry.h), shared by the grower and calculate_class_stats_cpp.
- Patch grower uses incremental moments (O(1) per candidate) and the shared
metric; thread method/batch_size through alloc_clumpy() and the evoland_db
binding; update unit tests to the C++ entry points.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189ymK6BeCwBC8iZ45CGuVb
…nterface cleanup
- avoid_aggregation (default TRUE for uPAM): deferred-write, all-or-nothing patch
growth that fails (allocating nothing) if a patch would merge with another or
cannot reach its sampled area; attempted cells are removed from the pool
(sampling without replacement). Replicates clumpy's GaussianPatcher.
- Auto-select the method from patch params (all mono-pixel -> uSAM, else uPAM)
instead of a user switch; uSAM is now strictly mono-pixel. The C++ keeps an
explicit method flag for tests/comparison.
- Expose patch-area distribution via area_dist ('lognormal' default, 'normal');
area_var is a variance (normal uses sd = sqrt(area_var)).
- allocate_clumpy_cpp drops ant_landscape (anterior snapshotted internally) and
from_classes (derived from trans_from).
- Rename eccentricity -> elongation everywhere (incl. alloc_params_clumpy_v
column), matching the thesis.
- Update unit + integration tests and RcppExports; extend dev verification note.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189ymK6BeCwBC8iZ45CGuVb
The thesis only uses the 'Multinomial Sampling Test (MuST)' name (Mazy App. 3.B); 'GART'/'generalized allocation rejection test' is local to the clumpy codebase, and 'rejection test' in the thesis refers only to Dinamica EGO's distinct two-stage process. What clumpy's GART computes is exactly MuST, so: - export the pivot test as must_cpp (was gart_cpp); rename the internal must_draw_one helper and all GART comments/docs to MuST. - the docstring notes the clumpy GART correspondence. - update RcppExports, unit tests, and R-layer docs accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189ymK6BeCwBC8iZ45CGuVb
…place For large rasters (8M+ cells) the dense n_cells x T potential matrix is the dominant memory cost. Pass the adjusted potentials as per-transition sparse R lists (prob_cell / prob_value) instead, and store them in C++ as SparseColumn (sorted cell indices + values, O(log nnz) lookup, ~12 B/nonzero). The grower is templated on a probability accessor so both the sparse allocator and the dense grow_patch_cpp test primitive share one implementation. Also: remove the now-unused shuffle_in_place (uSAM is mono-pixel/order-free; uPAM uses shuffle_pair); add a comment that cell indices are int (R 32-bit, caps rasters at ~2.1e9 cells). Update RcppExports and unit tests; the sparse subset test confirms only cells carrying a potential can transition. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189ymK6BeCwBC8iZ45CGuVb
The previous big-grid check sum(agg)<=sum(noagg) was not a valid invariant: both runs are quota-bounded uPAM (target_rate), so totals are ~equal not ordered; with avoidance, successful patches are full-size (all-or-nothing) so the last patch overshoots the quota more, and the FP-sensitive greedy growth diverges across compilers/arch (passed on x86/gcc by hitting the quota exactly, failed on arm/clang). Replace with a deterministic 1x5 case (forced potentials, area variance 0, no shuffle) where avoidance unambiguously rejects the middle merging patch: noagg=5, agg=4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189ymK6BeCwBC8iZ45CGuVb
Strict batch_size=1 re-scans the whole pool with MuST once per patch (O(#patches x pool)); at 200x200 that is ~22s vs ~0.08s for the batched path. Make batch_size=0 (new default) auto-scale to ~1% of each class's source pool, bounding the number of MuST passes (~100) so cost stays ~linear from 500k up to the int32 cell limit. Semantics: >0 explicit cap (1 = strict uPAM), <0 = all candidates in one pass, 0 = auto. Mirrors the reference's fraction-of-#J batch parameter (which itself defaults to strict, with no scaling). Thread the new default through alloc_clumpy()/one_period and the evoland_db binding; update docs and tests (deterministic row test pinned to explicit batch_size=1; add an auto-batch smoke test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189ymK6BeCwBC8iZ45CGuVb
…act checks
- Annotate the allocation backend with the relevant Mazy (2022) references so the
rationale is recoverable from the thesis: MuST inverse-CDF (App. 3.B.1,
eta_w), patch construction + elongation (App. 3.I, eq. 3.I.12), the 1/E(sigma)
pivot rarefaction (Fig. 3.2, eq. 3.11), the uPAM quota N_{u->v}=P(v|u)#J
(sec. 3.4.2 / App. 3.E.2), and patch-merging avoidance (sec. 3.2.4 / 3.4.2).
- Add an optional uniform-replay argument to must_cpp (split must_pick out of
must_draw_one) so an external uniform stream (e.g. numpy's, from the reference
clumpy GART) can be replayed for a deterministic, pixel-perfect cross-tool
comparison of the pivot test. Backwards-compatible (u defaults to NULL/RNG).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189ymK6BeCwBC8iZ45CGuVb
Replace terra::animate() in the stochastic-allocation-sensitivity vignette with a per-frame plot loop encoded into a browser-looping GIF via knitr's gifski animation hook, which (unlike interactive terra playback) survives into HTML output. Add gifski to Suggests. Also correct text that no longer matched the code: - runs inherit baseline alloc_params_t via run lineage (parent_id_run = 0), they are not duplicated per run; variation comes from per-run seeds - the allocation loop is a plain .mapply over runs, not a defensive signature-probing wrapper - the individual-realization section now describes the animation rather than side-by-side panels Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019E9zE7mVS9ijuYaJvkjy9r
The vignette intermittently failed in CI with "No model found for id_trans=6". Root cause: the rpart importance filter can return no score for a sparse transition whose tree has no splits, dropping it from trans_preds_t. Model fitting inner-joins viable transitions against trans_preds_t and silently skips it, but predict_trans_pot() loops over every viable transition and hard-errors. Whether the degenerate tree occurs depends on platform RNG, hence pass-locally / fail-in-CI. - vignette: after pruning predictors, demote any viable transition that retained no predictor so the viable set and the fitted models stay in lockstep; pass id_periods (not the partial-match id_period) to alloc_clumpy. - trans_pot_t.R: TODO flagging that the viable/modelled mismatch should be reconciled in the library rather than relying on callers. - alloc_params_t.R: drop the broken \link to the undocumented internal calculate_class_stats_cpp (Rd cross-reference WARNING) in favour of a code span; regenerate the matching man page. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019E9zE7mVS9ijuYaJvkjy9r
The previous vignette reconcile keyed on trans_preds_t, but the real failure mode is different: fit_full_model_worker() catches a training error (e.g. an rpart tree that never splits, or too few positive cases) and returns a row with learner_full = NULL rather than dropping it. predict_trans_pot() filters "learner_full is not null", so such a viable transition yields no model and the loop aborts with "No model found for id_trans=6". Whether training fails depends on platform RNG, hence pass-locally / fail-in-CI. (The is_viable subsetting itself is fine.) - predict_trans_pot(): up-front check that every viable transition has a non-null learner_full, erroring with an actionable message that names the offending transitions and shows how to demote them. Replaces the per-row "No model found" stop() as the primary guard (kept below as a fallback). - vignette: move the viability reconciliation after model fitting and key it on transitions that actually produced a usable model (learner_full is not null), which is the correct invariant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019E9zE7mVS9ijuYaJvkjy9r
The fetch-based reconcile demoted every transition: trans_models_t is partitioned by id_run and learner_full is a BLOB, and a plain (id_run = NULL) directory read does not round-trip "learner_full is not null", so the fetch came back empty and create_alloc_params_t() then hit "No viable transitions". - vignette: capture the trans_models_t object returned by fit_full_models() and demote transitions whose learner_full element is NULL, filtering the in-memory result directly instead of re-reading the partitioned table. - predict_trans_pot(): drop the db$fetch(...) incantation from the error message (unreliable under a plain read) in favour of a representation-neutral instruction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019E9zE7mVS9ijuYaJvkjy9r
Compute modeled_trans from the object returned by fit_full_models() before committing it (removes any residual by-reference risk) and pin the fit chunk seed. If no model trained at all, fail with the actual counts (rows, trained, viable, trans_preds, id lists) instead of the opaque downstream "No viable transitions found" error, so the cause is visible in CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019E9zE7mVS9ijuYaJvkjy9r
trained=0 for all 3 viable transitions in CI; the worker swallows the rpart training error as a (deferred, lost-on-abort) warning. Capture those warnings during fitting and include them in the diagnostic so the next CI log shows why every model fails to train. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019E9zE7mVS9ijuYaJvkjy9r
The captured fit warning was conclusive: "Package 'rpart' required but not installed for Learner 'classif.rpart'". rpart is a soft dependency of mlr3's classif.rpart learner and a "recommended" R package, but it was undeclared in DESCRIPTION. R CMD check restricts the vignette-build library to declared dependencies, so every classif.rpart fit failed, leaving all models NULL and aborting the stochastic vignette (evoland.qmd uses classif.rpart too). Tests pass because they use classif.featureless. - DESCRIPTION: add rpart to Suggests. - vignette: drop the diagnostic scaffolding (warning capture + degenerate- calibration stop) added while tracing this; keep the clean reconcile that demotes viable transitions whose full model did not train, which pairs with the new predict_trans_pot() guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019E9zE7mVS9ijuYaJvkjy9r
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
R CMD check (light) failed during package load because
evoland_db_views.Rattempted to add public methods that are now already declared inevoland_db.R. The failure was caused by duplicate R6 method registration at load time.R6 method override compatibility
evoland_db$set(...)calls inR/evoland_db_views.Rto explicitly allow replacing predeclared methods:trans_rates_dinamica_vadjusted_trans_pot_valloc_params_clumpy_vevoland_db.Ras the canonical method declaration surface while preserving view-backed implementations inevoland_db_views.R.Behavioral impact