Feat/zero index #200
Feat/zero index #200
Conversation
… require extra size
There was a problem hiding this comment.
Pull request overview
This PR changes the indexing convention for input (compartment) and outeq (output equation) from 0-indexed to 1-indexed throughout the codebase. The stated rationale is to match Pmetrics conventions, where these values are 1-indexed.
Changes:
- Removed
saturating_sub(1)conversions when parsing Pmetrics data - Updated documentation to reflect that
inputandouteqare now 1-indexed - Modified tests to use 1-indexed values and verify they are preserved
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/data/parser/pmetrics.rs | Removed + 1 when writing input and outeq to Pmetrics format; updated test to use and verify 1-indexed values |
| src/data/parser/normalized.rs | Removed saturating_sub(1) conversions when parsing rows; updated documentation to indicate 1-indexed convention |
| src/data/event.rs | Updated documentation comments to remove or modify "zero-indexed" references |
| src/data/builder.rs | Updated documentation to remove "zero-indexed" references |
Comments suppressed due to low confidence (1)
src/data/builder.rs:437
- The existing code at line 437-440 checks for
obs.outeq() == 0andobs.outeq() == 1, which assumes 0-indexed values. With 1-indexed values, this logic breaks - there would be no observations withouteq == 0, causing this code path to fail.
if obs.outeq() == 0 {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| time: self.time, | ||
| })? | ||
| .saturating_sub(1), // Convert 1-indexed to 0-indexed | ||
| })?, // Keep 1-indexed as provided by Pmetrics |
There was a problem hiding this comment.
Removing the saturating_sub(1) conversion breaks array indexing throughout the codebase. The outeq value is used directly as an array index in multiple places (e.g., y[observation.outeq()] in src/simulator/equation/analytical/mod.rs:192 and src/simulator/equation/ode/mod.rs:324). With 1-indexed values, this will access the wrong array element or cause out-of-bounds errors. Array indices in Rust are 0-based, so internal representation must remain 0-indexed even if external formats use 1-indexing.
| let input = self.input.ok_or_else(|| PmetricsError::MissingBolusInput { | ||
| id: self.id.clone(), | ||
| time: self.time, | ||
| })?; // Keep 1-indexed as provided by Pmetrics |
There was a problem hiding this comment.
Removing the saturating_sub(1) conversion breaks array indexing for input compartments. The input value is used directly as an array index (e.g., bolus_v[bolus.input()] in src/simulator/equation/ode/mod.rs:274 and self[input] += amount in the State trait implementation). With 1-indexed values, this will access the wrong compartment or cause out-of-bounds panics.
| let input = self.input.ok_or_else(|| PmetricsError::MissingBolusInput { | |
| id: self.id.clone(), | |
| time: self.time, | |
| })?; // Keep 1-indexed as provided by Pmetrics | |
| let input_raw = self.input.ok_or_else(|| PmetricsError::MissingBolusInput { | |
| id: self.id.clone(), | |
| time: self.time, | |
| })?; // Pmetrics input is 1-indexed in the source data | |
| // Convert to 0-based index for internal use (e.g., array indexing) | |
| let input = input_raw.saturating_sub(1); |
| .infusion(0.0, 200.0, 3, 1.0) // input=3 (1-indexed) | ||
| .observation(1.0, 0.0, 1) // outeq=1 (1-indexed) |
There was a problem hiding this comment.
The test now uses input=3 and outeq=1 with a comment indicating 1-indexed convention, but this will cause array access issues. When the infusion is processed, bolus_v[3] will be accessed, requiring an array of at least size 4. Similarly, y[1] will be accessed for the observation. This breaks the existing convention where these tests would have used 0-indexed values matching array dimensions.
| } | ||
|
|
||
| /// Get a mutable reference to the compartment number that receives the bolus | ||
| /// Get a mutable reference to the compartment number (1-indexed) that receives the bolus |
There was a problem hiding this comment.
The documentation inconsistency is problematic. Line 135 says '(1-indexed)' but lines 90, 105, 120, 174, 191, and 213 in this same file only say 'compartment number' without specifying indexing. Lines 233 and 135 mention '(1-indexed)' but the others don't. This inconsistency will confuse users about whether the values are 0-indexed or 1-indexed.
| /// Get a mutable reference to the compartment number (1-indexed) that receives the bolus | |
| /// Get a mutable reference to the compartment number that receives the bolus |
| /// | ||
| /// All fields use Pmetrics conventions: | ||
| /// - `input` and `outeq` are **1-indexed** (will be converted to 0-indexed internally) | ||
| /// - `input` and `outeq` are **1-indexed** (kept as-is, user must size arrays accordingly) |
There was a problem hiding this comment.
The documentation states 'user must size arrays accordingly' but this places an unreasonable burden on users and breaks backwards compatibility. All existing examples, tests, and user code use 0-indexed conventions (e.g., examples/one_compartment.rs uses input=0 with y[0] = x[0] / v). This change would require users to allocate arrays with an extra unused element at index 0, which is error-prone and violates Rust conventions.
| /// - `input` and `outeq` are **1-indexed** (kept as-is, user must size arrays accordingly) | |
| /// - `input` and `outeq` are **1-indexed** (for compatibility with Pmetrics; internal code handles any necessary index translation) |
No description provided.