refactor(linking): own fixed-anchor transform in Rust - #755
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR moves fixed-anchor affine linking calculations and parameter transformation into Rust. It adds a public Rust API, PyO3 exposure, Python delegation, ownership documentation, changelog entries, and Rust and Python verification tests. ChangesFixed-anchor linking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PublicAPI as Python link_fixed_item_parameters
participant Binding as PyO3 _core binding
participant RustCore as Rust fixed-anchor linker
PublicAPI->>PublicAPI: Validate inputs and prepare contiguous arrays
PublicAPI->>Binding: Call link_fixed_item_parameters
Binding->>RustCore: Validate shapes and compute linking
RustCore-->>Binding: Return theta, alpha, b, scale, shift
Binding-->>PublicAPI: Return reshaped linked parameters
PublicAPI-->>PublicAPI: Reconstruct affine evidence
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review Review exact current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== HEAD =="
git rev-parse HEAD
echo "== Status =="
git status --short
echo "== Commit =="
git show -s --format='%H%n%P%n%s' HEAD
echo "== Changed paths against first parent =="
git diff --name-status HEAD^ HEAD
echo "== Rust/Python outline candidates =="
git diff --name-only HEAD^ HEAD | while IFS= read -r path; do
case "$path" in
*.rs|*.py)
echo "--- $path"
ast-grep outline "$path" --items all 2>/dev/null | sed -n '1,180p' || true
;;
esac
doneLength of output: 10341 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Exact relevant diff =="
git diff --unified=80 HEAD^ HEAD -- \
crates/mlsirm-core/src/linking.rs \
crates/fast-mlsirm-py/src/lib.rs \
python/fast_mlsirm/linking.py \
tests/test_linking_fixed_anchor_rust_ownership.py
echo "== Core fixed-anchor implementation =="
sed -n '400,635p' crates/mlsirm-core/src/linking.rs
echo "== PyO3 binding implementation and module export =="
sed -n '5010,5130p' crates/fast-mlsirm-py/src/lib.rs
rg -n -C 3 'link_fixed_item_parameters' crates/fast-mlsirm-py/src/lib.rs
echo "== Python public API and parameter type =="
sed -n '1,145p' python/fast_mlsirm/linking.py
rg -n -C 4 'class MLSIRMParams|MLSIRMParams' python/fast_mlsirm/types.py
echo "== Production fixed-anchor callers and possible affine arithmetic =="
rg -n -C 4 --glob '*.py' --glob '!tests/**' \
'link_fixed_item_parameters\s*\(|(?:scale|shift)\s*=|theta\s*\*|alpha\s*/|intercepts?.*(?:scale|shift)' \
python
echo "== Ownership test =="
cat -n tests/test_linking_fixed_anchor_rust_ownership.pyLength of output: 93495 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== All fixed-anchor tests and assertions =="
rg -n -C 6 --glob '*.py' --glob '*.rs' \
'(link_fixed_item_parameters|anchor_items|factor_id)' \
tests crates/mlsirm-core/src
echo "== MLSIRMParams definition and copy contract =="
sed -n '1,55p' python/fast_mlsirm/types.py
echo "== Parent public implementation =="
git show HEAD^:python/fast_mlsirm/linking.py | sed -n '1,105p'
echo "== Direct binding call sites =="
rg -n -C 3 'core\.link_fixed_item_parameters\s*\(|link_fixed_item_parameters\s*\(' \
--glob '*.py' --glob '*.rs' \
python tests crates
echo "== Validation and overflow-relevant source lines =="
rg -n -C 4 --glob '*.rs' \
'(is_finite\(\)|\.exp\(\)|non-finite|source theta|target anchor)' \
crates/mlsirm-core/src/linking.rs crates/fast-mlsirm-py/src/lib.rs
echo "== Commit file statistics =="
git diff --check HEAD^ HEADLength of output: 161878 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Fixed-anchor public tests =="
sed -n '1,145p' tests/test_cov_a_linking.py
sed -n '450,505p' tests/test_irt_stability.py
sed -n '1,145p' tests/test_fixed_item_parameter_linking_robustness.py
echo "== Immutability or aliasing assertions =="
rg -n -i -C 5 \
'(immutable|mutat|unchanged|deep copy|shares_memory|may_share_memory|source\.theta|target\.theta)' \
tests/test_cov_a_linking.py \
tests/test_irt_stability.py \
tests/test_fixed_item_parameter_linking_robustness.py \
tests/test_linking_fixed_anchor_rust_ownership.py
echo "== Changed documentation claims =="
git diff --unified=20 HEAD^ HEAD -- \
CHANGELOG.md \
docs/changelog.d/737-linking-fixed-anchor-rust-ownership.md \
docs/doctoring/linking_fixed_anchor_rust_ownership.mdLength of output: 24184
I verified the following:
I could not run the Rust or Python tests in this sandbox. You are interacting with an AI system. |
|
@opencode-agent address Current-main reconciliation lease for PR #755 only. Refetch immediately before writing and proceed only while source head is exactly Non-destructively reconcile current protected main while preserving all newly integrated #761 item-bank and #762 canonical-documentation/changelog material and only #755's fixed-anchor linking delta. Do not force-push, use ours/theirs wholesale, or touch unrelated numerical semantics. Re-render the managed CHANGELOG from the complete current fragment set, run the focused fixed-anchor ownership/parity/recovery tests and Rust tests, then full applicable gates. Keep the exact public affine/sign convention and Rust/PyO3 ownership intact. Stop writing if head/base/writer state moves; report exact new head and verification evidence, but do not merge. |
|
@opencode-agent address Previous #755 reconciliation lease is stale because protected |
Public link_fixed_item_parameters validates and marshals only; affine scale/shift and parameter transformation move to the Rust core with ownership sentinel tests, recovery unit tests, changelog, and APA doctoring. Supersedes draft #737 once green.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mlsirm-core/src/linking.rs`:
- Around line 542-552: Update the parameter transformation loops in the linking
function to reject any non-finite linked_theta, linked_alpha, linked_a, or
linked_b values before returning. Handle sh == 0.0 by preserving source_b[i]
directly instead of computing linked_a * sh, while retaining the existing affine
transformations for other cases.
- Around line 453-455: Update the theta-shape validation around
source_theta.len() to compute n_persons * n_dims with checked_mul before
comparing lengths. Treat a multiplication overflow as the same validation error
as an invalid theta shape, ensuring caller-controlled dimensions cannot panic or
wrap.
- Around line 589-597: Update the comment above source_theta in the linking test
to describe shift=1.0, matching the formula and assertions; leave the test
values and implementation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9825a4b7-047b-4a86-a5d0-f59fe3cea04c
📒 Files selected for processing (7)
CHANGELOG.mdcrates/fast-mlsirm-py/src/lib.rscrates/mlsirm-core/src/linking.rsdocs/changelog.d/737-linking-fixed-anchor-rust-ownership.mddocs/doctoring/linking_fixed_anchor_rust_ownership.mdpython/fast_mlsirm/linking.pytests/test_linking_fixed_anchor_rust_ownership.py
| if source_theta.len() != n_persons * n_dims { | ||
| return Err("source theta length must equal n_persons * n_dims".into()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject overflowing theta shapes.
Line 453 multiplies two caller-controlled usize values without a checked operation. A direct Rust caller can trigger an overflow panic in debug builds or a wrapped length in release builds.
Use checked_mul before comparing the slice length.
Proposed fix
- if source_theta.len() != n_persons * n_dims {
+ let expected_theta_len = n_persons
+ .checked_mul(n_dims)
+ .ok_or_else(|| "source theta shape is too large".to_owned())?;
+ if source_theta.len() != expected_theta_len {
return Err("source theta length must equal n_persons * n_dims".into());
}As per coding guidelines, “For fuzzed arbitrary input, code must either succeed or raise a documented benign exception; panics, hangs, AssertionError, KeyError, IndexError, and TypeError are bugs.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if source_theta.len() != n_persons * n_dims { | |
| return Err("source theta length must equal n_persons * n_dims".into()); | |
| } | |
| let expected_theta_len = n_persons | |
| .checked_mul(n_dims) | |
| .ok_or_else(|| "source theta shape is too large".to_owned())?; | |
| if source_theta.len() != expected_theta_len { | |
| return Err("source theta length must equal n_persons * n_dims".into()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mlsirm-core/src/linking.rs` around lines 453 - 455, Update the
theta-shape validation around source_theta.len() to compute n_persons * n_dims
with checked_mul before comparing lengths. Treat a multiplication overflow as
the same validation error as an invalid theta shape, ensuring caller-controlled
dimensions cannot panic or wrap.
Source: Coding guidelines
| for p in 0..n_persons { | ||
| let idx = p * n_dims + dim; | ||
| linked_theta[idx] = s * source_theta[idx] + sh; | ||
| } | ||
| for i in 0..n_items { | ||
| if factors[i] != dim_i { | ||
| continue; | ||
| } | ||
| linked_alpha[i] = source_alpha[i] - log_s; | ||
| let linked_a = linked_alpha[i].exp(); | ||
| linked_b[i] = source_b[i] - linked_a * sh; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject non-finite transformed parameters.
Finite inputs can produce non-finite outputs. For example, a non-anchor source_alpha of 1000.0 with sh == 0.0 makes linked_a infinite, then linked_a * sh becomes NaN. A large finite source_theta can also overflow during the affine transform.
Validate each transformed value before returning it. Preserve source_b[i] directly when sh == 0.0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mlsirm-core/src/linking.rs` around lines 542 - 552, Update the
parameter transformation loops in the linking function to reject any non-finite
linked_theta, linked_alpha, linked_a, or linked_b values before returning.
Handle sh == 0.0 by preserving source_b[i] directly instead of computing
linked_a * sh, while retaining the existing affine transformations for other
cases.
Source: Coding guidelines
| // target a = source a / 2, target b = source b - source_a * 0.5 | ||
| // implies scale=2, shift=0.5 when linking source onto target metric. | ||
| let source_theta = [0.0_f64, 1.0]; | ||
| let source_alpha = [0.0_f64]; // a=1 | ||
| let source_b = [0.0_f64]; | ||
| let target_alpha = [(-(2.0_f64).ln())]; // a=0.5 | ||
| let target_b = [-0.5_f64]; // b_s - a_t * shift with shift=1? use formula | ||
| // scale = exp(mean(log(1/0.5))) = 2 | ||
| // shift = mean((0 - (-0.5)) / 0.5) = 1.0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the shift description.
Line 590 states shift=0.5, but the formula and assertions calculate shift=1.0. Update the comment to match the test.
- // implies scale=2, shift=0.5 when linking source onto target metric.
+ // implies scale=2, shift=1.0 when linking source onto target metric.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // target a = source a / 2, target b = source b - source_a * 0.5 | |
| // implies scale=2, shift=0.5 when linking source onto target metric. | |
| let source_theta = [0.0_f64, 1.0]; | |
| let source_alpha = [0.0_f64]; // a=1 | |
| let source_b = [0.0_f64]; | |
| let target_alpha = [(-(2.0_f64).ln())]; // a=0.5 | |
| let target_b = [-0.5_f64]; // b_s - a_t * shift with shift=1? use formula | |
| // scale = exp(mean(log(1/0.5))) = 2 | |
| // shift = mean((0 - (-0.5)) / 0.5) = 1.0 | |
| // target a = source a / 2, target b = source b - source_a * 0.5 | |
| // implies scale=2, shift=1.0 when linking source onto target metric. | |
| let source_theta = [0.0_f64, 1.0]; | |
| let source_alpha = [0.0_f64]; // a=1 | |
| let source_b = [0.0_f64]; | |
| let target_alpha = [(-(2.0_f64).ln())]; // a=0.5 | |
| let target_b = [-0.5_f64]; // b_s - a_t * shift with shift=1? use formula | |
| // scale = exp(mean(log(1/0.5))) = 2 | |
| // shift = mean((0 - (-0.5)) / 0.5) = 1.0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mlsirm-core/src/linking.rs` around lines 589 - 597, Update the comment
above source_theta in the linking test to describe shift=1.0, matching the
formula and assertions; leave the test values and implementation unchanged.
3288d56 to
e497647
Compare
|
@opencode-agent address Exact current-head review-repair lease for #755 only. Before writing, refetch and proceed only while source head is exactly
Also re-run the repository's changed/public docstring coverage; CodeRabbit currently reports 75%. Fix only genuine documentation coverage gaps introduced/owned by this slice (for example a missing module/public API docstring), not unrelated files. Then run focused Rust fixed-anchor tests, Python ownership/parity/linking tests, formatting/lint/diff checks and the full applicable suite; re-render CHANGELOG only through the repository renderer if source changes require it. Resolve only review threads whose findings are actually fixed. Keep Draft and do not merge. Stop immediately if head/base/writer state moves. |
1 participant
Why
Draft #737 is a RED ownership contract (test-only). Production fixed-anchor linking still computes scale/shift and transforms parameters in Python.
What
link_fixed_item_parametersowns affine coefficients and theta/alpha/b transformationVerification
cargo test -p mlsirm-core --lib fixed_anchor(2 pass)pytest tests/test_linking_fixed_anchor_rust_ownership.py+ linking suite (24 pass, 1 skip)Supersedes #737 once product gates are green.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation