Skip to content

Feat/zero index #200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Siel wants to merge 1 commit into
base: feat/normalized-row-api
Choose a base branch
from
Open

Feat/zero index #200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/data/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl SubjectBuilder {
///
/// * `time` - Time of the bolus dose
/// * `amount` - Amount of drug administered
/// * `input` - The compartment number (zero-indexed) receiving the dose
/// * `input` - The compartment number receiving the dose
pub fn bolus(self, time: f64, amount: f64, input: usize) -> Self {
let bolus = Bolus::new(time, amount, input, self.current_occasion.index());
let event = Event::Bolus(bolus);
Expand All @@ -79,7 +79,7 @@ impl SubjectBuilder {
///
/// * `time` - Start time of the infusion
/// * `amount` - Total amount of drug to be administered
/// * `input` - The compartment number (zero-indexed) receiving the dose
/// * `input` - The compartment number receiving the dose
/// * `duration` - Duration of the infusion in time units
pub fn infusion(self, time: f64, amount: f64, input: usize, duration: f64) -> Self {
let infusion = Infusion::new(time, amount, input, duration, self.current_occasion.index());
Expand All @@ -93,8 +93,7 @@ impl SubjectBuilder {
///
/// * `time` - Time of the observation
/// * `value` - Observed value (e.g., drug concentration)
/// * `outeq` - Output equation number (zero-indexed) corresponding to this observation
/// * `errorpoly` - Error polynomial coefficients (c0, c1, c2, c3)
/// * `outeq` - Output equation number corresponding to this observation
pub fn observation(self, time: f64, value: f64, outeq: usize) -> Self {
let observation = Observation::new(
time,
Expand Down
25 changes: 13 additions & 12 deletions src/data/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl Bolus {
///
/// * `time` - Time of the bolus dose
/// * `amount` - Amount of drug administered
/// * `input` - The compartment number (zero-indexed) receiving the dose
/// * `input` - The compartment number receiving the dose
pub fn new(time: f64, amount: f64, input: usize, occasion: usize) -> Self {
Bolus {
time,
Expand All @@ -102,7 +102,7 @@ impl Bolus {
self.amount
}

/// Get the compartment number (zero-indexed) that receives the bolus
/// Get the compartment number that receives the bolus
pub fn input(&self) -> usize {
self.input
}
Expand All @@ -117,7 +117,7 @@ impl Bolus {
self.amount = amount;
}

/// Set the compartment number (zero-indexed) that receives the bolus
/// Set the compartment number that receives the bolus
pub fn set_input(&mut self, input: usize) {
self.input = input;
}
Expand All @@ -132,7 +132,7 @@ impl Bolus {
&mut self.amount
}

/// 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
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
/// 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

Copilot uses AI. Check for mistakes.
pub fn mut_input(&mut self) -> &mut usize {
&mut self.input
}
Expand Down Expand Up @@ -171,7 +171,7 @@ impl Infusion {
///
/// * `time` - Start time of the infusion
/// * `amount` - Total amount of drug to be administered
/// * `input` - The compartment number (zero-indexed) receiving the dose
/// * `input` - The compartment number receiving the dose
/// * `duration` - Duration of the infusion in time units
pub fn new(time: f64, amount: f64, input: usize, duration: f64, occasion: usize) -> Self {
Infusion {
Expand All @@ -188,7 +188,7 @@ impl Infusion {
self.amount
}

/// Get the compartment number (zero-indexed) that receives the infusion
/// Get the compartment number that receives the infusion
pub fn input(&self) -> usize {
self.input
}
Expand All @@ -210,7 +210,7 @@ impl Infusion {
self.amount = amount;
}

/// Set the compartment number (zero-indexed) that receives the infusion
/// Set the compartment number that receives the infusion
pub fn set_input(&mut self, input: usize) {
self.input = input;
}
Expand All @@ -230,7 +230,7 @@ impl Infusion {
&mut self.amount
}

/// Set the compartment number (zero-indexed) that receives the infusion
/// Get a mutable reference to the compartment number (1-indexed) that receives the infusion
pub fn mut_input(&mut self) -> &mut usize {
&mut self.input
}
Expand Down Expand Up @@ -284,9 +284,10 @@ impl Observation {
///
/// * `time` - Time of the observation
/// * `value` - Observed value (e.g., drug concentration)
/// * `outeq` - Output equation number (zero-indexed) corresponding to this observation
/// * `outeq` - Output equation number corresponding to this observation
/// * `errorpoly` - Optional error polynomial coefficients (c0, c1, c2, c3)
/// * `ignore` - Whether to ignore this observation in calculations
/// * `occasion` - Occasion index
/// * `censoring` - Censoring type for this observation
pub(crate) fn new(
time: f64,
value: Option<f64>,
Expand Down Expand Up @@ -315,7 +316,7 @@ impl Observation {
self.value
}

/// Get the output equation number (zero-indexed) corresponding to this observation
/// Get the output equation number corresponding to this observation
pub fn outeq(&self) -> usize {
self.outeq
}
Expand All @@ -337,7 +338,7 @@ impl Observation {
self.value = value;
}

/// Set the output equation number (zero-indexed) corresponding to this observation
/// Set the output equation number corresponding to this observation
pub fn set_outeq(&mut self, outeq: usize) {
self.outeq = outeq;
}
Expand Down
32 changes: 14 additions & 18 deletions src/data/parser/normalized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ use std::collections::HashMap;
/// # Fields
///
/// 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)
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
/// - `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)

Copilot uses AI. Check for mistakes.
/// - `evid`: 0=observation, 1=dose, 4=reset/new occasion
/// - `addl`: positive=forward in time, negative=backward in time
///
Expand Down Expand Up @@ -92,11 +92,11 @@ pub struct NormalizedRow {
pub addl: Option<i64>,
/// Interdose interval for ADDL
pub ii: Option<f64>,
/// Input compartment (1-indexed in Pmetrics convention)
/// Input compartment
pub input: Option<usize>,
/// Observed value (for EVID=0)
pub out: Option<f64>,
/// Output equation number (1-indexed)
/// Output equation number
pub outeq: Option<usize>,
/// Censoring indicator
pub cens: Option<Censor>,
Expand Down Expand Up @@ -201,22 +201,18 @@ impl NormalizedRow {
.ok_or_else(|| PmetricsError::MissingObservationOuteq {
id: self.id.clone(),
time: self.time,
})?
.saturating_sub(1), // Convert 1-indexed to 0-indexed
})?, // Keep 1-indexed as provided by Pmetrics
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
self.get_errorpoly(),
0, // occasion set later
self.cens.unwrap_or(Censor::None),
)));
}
1 | 4 => {
// Dosing event (1) or reset with dose (4)
let input_0indexed = self
.input
.ok_or_else(|| PmetricsError::MissingBolusInput {
id: self.id.clone(),
time: self.time,
})?
.saturating_sub(1); // Convert 1-indexed to 0-indexed
let input = self.input.ok_or_else(|| PmetricsError::MissingBolusInput {
id: self.id.clone(),
time: self.time,
})?; // Keep 1-indexed as provided by Pmetrics
Comment on lines +212 to +215
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.

let event = if self.dur.unwrap_or(0.0) > 0.0 {
// Infusion
Expand All @@ -227,7 +223,7 @@ impl NormalizedRow {
id: self.id.clone(),
time: self.time,
})?,
input_0indexed,
input,
self.dur.ok_or_else(|| PmetricsError::MissingInfusionDur {
id: self.id.clone(),
time: self.time,
Expand All @@ -242,7 +238,7 @@ impl NormalizedRow {
id: self.id.clone(),
time: self.time,
})?,
input_0indexed,
input,
0,
))
};
Expand Down Expand Up @@ -388,7 +384,7 @@ impl NormalizedRowBuilder {
/// Set the input compartment (1-indexed)
///
/// Required for EVID=1 (dosing events).
/// Will be converted to 0-indexed internally.
/// Kept as 1-indexed; user must size state arrays accordingly.
pub fn input(mut self, input: usize) -> Self {
self.row.input = Some(input);
self
Expand Down Expand Up @@ -577,7 +573,7 @@ mod tests {
Event::Observation(obs) => {
assert_eq!(obs.time(), 1.0);
assert_eq!(obs.value(), Some(25.5));
assert_eq!(obs.outeq(), 0); // Converted to 0-indexed
assert_eq!(obs.outeq(), 1); // Kept as 1-indexed
}
_ => panic!("Expected observation event"),
}
Expand All @@ -598,7 +594,7 @@ mod tests {
Event::Bolus(bolus) => {
assert_eq!(bolus.time(), 0.0);
assert_eq!(bolus.amount(), 100.0);
assert_eq!(bolus.input(), 0); // Converted to 0-indexed
assert_eq!(bolus.input(), 1); // Kept as 1-indexed
}
_ => panic!("Expected bolus event"),
}
Expand All @@ -621,7 +617,7 @@ mod tests {
assert_eq!(inf.time(), 0.0);
assert_eq!(inf.amount(), 100.0);
assert_eq!(inf.duration(), 2.0);
assert_eq!(inf.input(), 0);
assert_eq!(inf.input(), 1); // Kept as 1-indexed
}
_ => panic!("Expected infusion event"),
}
Expand Down
12 changes: 6 additions & 6 deletions src/data/parser/pmetrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ impl Data {
let value = obs
.value()
.map_or_else(|| ".".to_string(), |v| v.to_string());
let outeq = (obs.outeq() + 1).to_string();
let outeq = obs.outeq().to_string();
let censor = match obs.censoring() {
Censor::None => "0".to_string(),
Censor::BLOQ => "1".to_string(),
Expand Down Expand Up @@ -372,7 +372,7 @@ impl Data {
&inf.amount().to_string(),
&".".to_string(),
&".".to_string(),
&(inf.input() + 1).to_string(),
&inf.input().to_string(),
&".".to_string(),
&".".to_string(),
&".".to_string(),
Expand All @@ -393,7 +393,7 @@ impl Data {
&bol.amount().to_string(),
&".".to_string(),
&".".to_string(),
&(bol.input() + 1).to_string(),
&bol.input().to_string(),
&".".to_string(),
&".".to_string(),
&".".to_string(),
Expand Down Expand Up @@ -466,8 +466,8 @@ mod tests {
#[test]
fn write_pmetrics_preserves_infusion_input() {
let subject = Subject::builder("writer")
.infusion(0.0, 200.0, 2, 1.0)
.observation(1.0, 0.0, 0)
.infusion(0.0, 200.0, 3, 1.0) // input=3 (1-indexed)
.observation(1.0, 0.0, 1) // outeq=1 (1-indexed)
Comment on lines +469 to +470
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
.build();
let data = Data::new(vec![subject]);

Expand All @@ -485,7 +485,7 @@ mod tests {
.find(|record| record.get(3) != Some("0"))
.expect("infusion row missing");

assert_eq!(infusion_row.get(7), Some("3"));
assert_eq!(infusion_row.get(7), Some("3")); // Written as-is (1-indexed)
}

#[test]
Expand Down
Loading