Only zero-index Pmetrics format #197
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to consolidate index conversion from 1-indexed to 0-indexed at the Pmetrics parser level rather than during event conversion. The Pmetrics format uses 1-indexed values for compartment and equation indices, while the internal representation uses 0-indexed values.
Changes:
- Removed index conversion logic from
row.rsforouteqandinputfields - Added 0-indexing conversion for
outeqin the Pmetrics parser - Simplified variable naming by removing
_0indexedsuffix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/data/row.rs | Removed .saturating_sub(1) calls that converted indices from 1-indexed to 0-indexed during event creation, and removed _0indexed variable name suffix |
| src/data/parser/pmetrics.rs | Added .saturating_sub(1) conversion for outeq field when reading Pmetrics format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .out | ||
| .and_then(|v| if v == -99.0 { None } else { Some(v) }), | ||
| outeq: self.outeq, | ||
| outeq: self.outeq.map(|v| v.saturating_sub(1)), |
There was a problem hiding this comment.
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))
| time: self.time, | ||
| })? | ||
| .saturating_sub(1), // Convert 1-indexed to 0-indexed | ||
| })?, // Convert 1-indexed to 0-indexed |
There was a problem hiding this comment.
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.
| })?, // Convert 1-indexed to 0-indexed | |
| })?, |
| let input = self.input.ok_or_else(|| DataError::MissingBolusInput { | ||
| id: self.id.clone(), | ||
| time: self.time, | ||
| })?; |
There was a problem hiding this comment.
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));
| 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, | |
| })?; |
No description provided.