Skip to content

Only zero-index Pmetrics format #197

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
mhovd wants to merge 1 commit into
base: norm-row-suggestions
Choose a base branch
from
Open

Only zero-index Pmetrics format #197

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
2 changes: 1 addition & 1 deletion src/data/parser/pmetrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl Row {
out: self
.out
.and_then(|v| if v == -99.0 { None } else { Some(v) }),
outeq: self.outeq,
outeq: self.outeq.map(|v| v.saturating_sub(1)),
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The input field should also be converted from 1-indexed to 0-indexed when reading Pmetrics format, similar to how outeq is now being converted on line 142. The Pmetrics format uses 1-indexed values for both fields (as evidenced by the write_pmetrics method at lines 341 and 362 that adds 1 to both when writing), but the internal representation is 0-indexed. The conversion for input was removed from row.rs but not added here in the parser, which will cause incorrect compartment indexing. Line 137 should be changed to: input: self.input.map(|v| v.saturating_sub(1))

Copilot uses AI. Check for mistakes.
cens: self.cens,
c0: self.c0,
c1: self.c1,
Expand Down
18 changes: 7 additions & 11 deletions src/data/row.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,22 +187,18 @@ impl DataRow {
.ok_or_else(|| DataError::MissingObservationOuteq {
id: self.id.clone(),
time: self.time,
})?
.saturating_sub(1), // Convert 1-indexed to 0-indexed
})?, // Convert 1-indexed to 0-indexed
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The comment "Convert 1-indexed to 0-indexed" is now outdated and misleading since the conversion has been moved to the Pmetrics parser. This comment should be removed as the DataRow now expects to receive already 0-indexed values.

Suggested change
})?, // Convert 1-indexed to 0-indexed
})?,

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(|| DataError::MissingBolusInput {
id: self.id.clone(),
time: self.time,
})?
.saturating_sub(1); // Convert 1-indexed to 0-indexed
let input = self.input.ok_or_else(|| DataError::MissingBolusInput {
id: self.id.clone(),
time: self.time,
})?;
Comment on lines +198 to +201
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The DataRow builder API is documented to accept 1-indexed values for input (see lines 369-372) and convert them internally, but this conversion was removed. This breaks the builder API contract. Either the conversion must be restored in the builder (by having the builder itself convert to 0-indexed before storing in DataRow), or the builder's documentation and all its usage must be updated to reflect that it now expects 0-indexed values. Given that the PR doesn't update documentation or usages, the likely intent is to restore the conversion. The conversion should happen in the builder's input() method at line 374, changing it to: self.row.input = Some(input.saturating_sub(1));

Suggested change
let input = self.input.ok_or_else(|| DataError::MissingBolusInput {
id: self.id.clone(),
time: self.time,
})?;
// Convert 1-indexed input (Pmetrics convention) to 0-indexed internally
let input = self
.input
.map(|i| i.saturating_sub(1))
.ok_or_else(|| DataError::MissingBolusInput {
id: self.id.clone(),
time: self.time,
})?;

Copilot uses AI. Check for mistakes.

let event = if self.dur.unwrap_or(0.0) > 0.0 {
// Infusion
Expand All @@ -212,7 +208,7 @@ impl DataRow {
id: self.id.clone(),
time: self.time,
})?,
input_0indexed,
input,
self.dur.ok_or_else(|| DataError::MissingInfusionDur {
id: self.id.clone(),
time: self.time,
Expand All @@ -227,7 +223,7 @@ impl DataRow {
id: self.id.clone(),
time: self.time,
})?,
input_0indexed,
input,
0,
))
};
Expand Down
Loading