fix(upstream): stop FixDupsIter replaying the final training row

[nh13]

Summary

FixDupsIter (behind RMITrainingData::iter()) re-emitted the already-yielded final row in its terminal branch (None => self.last_item.take()), so iter() produced len()+1 items for any input — the last (key, offset) pair appeared twice.

Unlike most of the latent upstream bugs tracked in #3, this one is on the active trainer path: train_with_config fits every L2 leaf LinearModel (trainer.rs:335) and the routing LinearModel (trainer.rs:478) via data.iter() (LinearModel::new, linear.rs:177), so each slr fit double-weighted its final point. The bias is small for large leaves and grows as leaves get small. (LowerBoundCorrection::new also iterates data.iter(), but the phantom only inflated longest_run by 1 — which the active path doesn't use — and left first/last/next/prev unchanged.)

Fix

Return None at exhaustion instead of replaying. Also:

• Document FixDupsIter's contract (collapse duplicate-key runs onto the first occurrence's offset; yield exactly one item per input row).
• Add regression tests: iter() reproduces the input one-to-one (no trailing replay), and a duplicate-key run collapses onto the first offset while keeping len() unchanged.
• Drop the now-stale "FixDupsIter adds a trailing replay" workaround note in the lower-bound single_run_reports_full_length test (that test derives its expected count from data.iter(), so it still passes).

Validation

• Full cargo test suite passes (no existing golden-value test depended on the phantom row — the perfect-linear-fit trainer tests are unaffected, since duplicating a point already on the line doesn't change the fit).
• cargo clippy -p prmi --all-targets clean.

Context

Inherited verbatim from learnedsystems/RMI; also fixed upstream in learnedsystems#15 (which revived that repo's bit-rotted unit tests — the failures are what surfaced this bug). Tracked in #3.

Refs #3

Summary by CodeRabbit

• Bug Fixes
  • Fixed an iterator behavior that was incorrectly double-weighting the final data point during model training, which could affect training accuracy and downstream model performance. The iterator now correctly yields exactly one item per input pair without re-emitting the final element.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 58ab0d62-25fb-4aa5-9ae4-fe8380878a85

📥 Commits

Reviewing files that changed from the base of the PR and between 8ec4586 and 293455f.

📒 Files selected for processing (2)

• prmi/src/upstream/models/mod.rs
• prmi/src/upstream/train/lower_bound_correction.rs

---

Walkthrough

FixDupsIter::next() previously returned last_item.take() on exhaustion, emitting an extra trailing element and double-weighting the final point in SLR/CDF training. The fix returns None instead. Documentation and unit tests are added; a downstream test comment referencing the old behavior is updated.

Changes

FixDupsIter exhaustion fix

Layer / File(s)	Summary
FixDupsIter doc, exhaustion fix, and tests prmi/src/upstream/models/mod.rs	Adds doc comment on FixDupsIter semantics; changes the exhaustion branch in next() from last_item.take() to None; adds fix_dups_iter_tests asserting iter().count() == len() and duplicate-key offset collapsing.
Downstream test comment update prmi/src/upstream/train/lower_bound_correction.rs	Removes stale comment referencing the trailing-replay implementation detail from single_run_reports_full_length.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• Upstream RMI primitives: latent bugs to revisit (deferred from #2) #3: The FixDupsIter exhaustion fix directly addresses the double-weighting of the final point in downstream SLR/CDF training documented in that issue, and the lower_bound_correction.rs comment update reflects the removed behavior.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main bug fix: FixDupsIter was replaying the final training row, causing double-weighted final points in downstream SLR fits.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/fixdups-iter-replay

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.