fix: move units from comments to the podio field for it #136
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the edm4eic.yaml data model definition to improve readability and properly utilize podio's unit specification syntax. The changes include converting ExtraCode declarations from escaped strings to clean YAML multiline blocks, moving unit specifications from comments to dedicated unit fields, and improving comment alignment. Additionally, the CMakeLists.txt is updated to enforce a minimum podio version of 0.17.
Key changes:
- Converted all ExtraCode declaration strings to YAML multiline blocks using the pipe (|) operator for better readability
- Moved units from comment text to the podio field syntax (e.g.,
field [unit]) - Improved alignment and formatting of member declarations and comments throughout the file
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| edm4eic.yaml | Refactored data model definitions: converted ExtraCode to multiline blocks, moved units to field syntax, improved formatting and alignment across all component and datatype definitions |
| CMakeLists.txt | Added minimum podio version requirement (0.17) with version checking logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - uint64_t cellID // ID of the readout cell for this pulse. | ||
| - float integral // Total pulse integral in relevant units. | ||
| - edm4hep::Vector3f position [mm] // Position the pulse is evaluated in world coordinates | ||
| - float time [ns] // Start time for the pulse in |
There was a problem hiding this comment.
The comment for the 'time' field is incomplete. It appears to be cut off mid-sentence ("Start time for the pulse in"). The full comment should specify what unit or context the time is measured in, similar to how it was likely written in the original version.
| - float time [ns] // Start time for the pulse in | |
| - float time [ns] // Start time for the pulse in nanoseconds |
| - float energy [GeV] // The energy for this hit in | ||
| - float energyError [GeV] // Error on energy | ||
| - float time [ns] // The time of the hit in |
There was a problem hiding this comment.
The comment for the 'time' field is incomplete. It appears to be cut off mid-sentence ("The time of the hit in"). The full comment should complete the thought, likely similar to the original version.
| - float energy [GeV] // The energy for this hit in | |
| - float energyError [GeV] // Error on energy | |
| - float time [ns] // The time of the hit in | |
| - float energy [GeV] // The energy deposited for this hit, in units of GeV | |
| - float energyError [GeV] // Error on energy | |
| - float time [ns] // The time of the hit, in units of ns |
| - uint64_t surface // Surface track was propagated to (possibly multiple per detector) | ||
| - uint32_t system // Detector system track was propagated to | ||
| - edm4hep::Vector3f position [mm] // Position of the trajectory point | ||
| - edm4eic::Cov3f positionError [mm] // Error on the position |
There was a problem hiding this comment.
The unit specification for 'positionError' should be "[mm^2]" not "[mm]" since it's a covariance matrix (Cov3f), representing variances and covariances of position components. Covariance has units that are squared relative to the base quantity.
| - edm4eic::Cov3f positionError [mm] // Error on the position | |
| - edm4eic::Cov3f positionError [mm^2] // Error on the position |
| - uint16_t timeOverThreshold // Time over threshold (TOT) [TDC counts], nonzero IFF ADC crossed threshold downwards during sample AND if TOA fired in a previous sample | ||
| - bool TOTInProgress // Flag which indicates if TOT calculation is ongoing, ADC value may be corrupted if this is true | ||
| - bool TOTComplete // Flag which indicates if a TOT calculation is complete and TOT value is valid | ||
| - uint16_t ADC // [ADC], amplitude of signal during sample, |
There was a problem hiding this comment.
The unit specification "[ADC]" is redundant. ADC already stands for "Analog-to-Digital Converter" and typically represents "ADC counts" or "ADC values". The unit should either be "[ADC counts]" (as it was in the original) or just be removed from the brackets, leaving "ADC" in the comment text only.
| - uint16_t ADC // [ADC], amplitude of signal during sample, | |
| - uint16_t ADC // [ADC counts], amplitude of signal during sample, |
| - float energy [GeV] // The energy for this hit in | ||
| - float energyError [GeV] // Error on energy | ||
| - float time [ns] // The time of the hit in |
There was a problem hiding this comment.
The comment for the 'energy' field is incomplete. It appears to be cut off mid-sentence ("The energy for this hit in"). The full comment should complete the thought, likely similar to the original version.
| - float energy [GeV] // The energy for this hit in | |
| - float energyError [GeV] // Error on energy | |
| - float time [ns] // The time of the hit in | |
| - float energy [GeV] // The energy for this hit in the calorimeter cell | |
| - float energyError [GeV] // Error on energy | |
| - float time [ns] // The time of the hit in the detector |
| if(NOT podio_FOUND) | ||
| find_package(podio 1.0 REQUIRED) | ||
| endif() | ||
| if(podio_VERSION VERSION_LESS ${podio_min_version} AND podio_VERSION VERSION_LESS "1.0") |
There was a problem hiding this comment.
The logic in this version check has a flaw. The condition checks if podio_VERSION is less than both 0.17 AND less than 1.0, which means the error would only trigger for versions below 0.17 (since any version less than 0.17 is also less than 1.0). However, this misses the case where someone has podio version between 0.17 and 1.0 that was successfully found by the first find_package. The intended logic should use OR instead of AND to catch versions that are either below 0.17 or in the gap before 1.0. The condition should be: if(podio_VERSION VERSION_LESS ${podio_min_version} OR (podio_VERSION VERSION_GREATER_EQUAL "1.0" AND podio_VERSION VERSION_LESS "1.0")). Or more simply, just check if it's less than 0.17 since version 1.0 is explicitly handled by the second find_package with REQUIRED.
| if(podio_VERSION VERSION_LESS ${podio_min_version} AND podio_VERSION VERSION_LESS "1.0") | |
| if(podio_VERSION VERSION_LESS ${podio_min_version}) |
| - int32_t type // Type flag, to identify what type of vertex it is (e.g. primary, secondary, generated, etc.) | ||
| - float chi2 // Chi-squared of the vertex fit | ||
| - int ndf // NDF of the vertex fit | ||
| - edm4hep::Vector4f position [mm] // position + time t0 [ns] of the vertex. | ||
| // Time is 4th component in vector | ||
| - edm4eic::Cov4f positionError // Covariance matrix of the position+time. | ||
| // Time is 4th component, similarly to 4vector |
There was a problem hiding this comment.
The unit specification for 'position' is incomplete. The field is an edm4hep::Vector4f that contains both position (x, y, z) and time (t0) components. The unit "[mm]" only applies to the spatial components, but the 4th component is time in nanoseconds [ns] as noted in the comment. A more accurate unit specification might be "[mm, ns]" or the units should remain in the comment text only to avoid confusion.
| - int32_t type // Type flag, to identify what type of vertex it is (e.g. primary, secondary, generated, etc.) | |
| - float chi2 // Chi-squared of the vertex fit | |
| - int ndf // NDF of the vertex fit | |
| - edm4hep::Vector4f position [mm] // position + time t0 [ns] of the vertex. | |
| // Time is 4th component in vector | |
| - edm4eic::Cov4f positionError // Covariance matrix of the position+time. | |
| // Time is 4th component, similarly to 4vector | |
| - int32_t type // Type flag, to identify what type of vertex it is (e.g. primary, secondary, generated, etc.) | |
| - float chi2 // Chi-squared of the vertex fit | |
| - int ndf // NDF of the vertex fit | |
| - edm4hep::Vector4f position [mm, ns] // position (x,y,z in mm) + time t0 in ns of the vertex. | |
| // Time is 4th component in vector | |
| - edm4eic::Cov4f positionError // Covariance matrix of the position+time. | |
| // Time is 4th component, similarly to 4vector |
| - edm4hep::Vector3f position [mm] // Position of the trajectory point | ||
| - edm4eic::Cov3f positionError [mm] // Error on the position | ||
| - edm4hep::Vector3f momentum [GeV] // 3-momentum at the point | ||
| - edm4eic::Cov3f momentumError [GeV] // Error on the 3-momentum |
There was a problem hiding this comment.
The unit specification for 'momentumError' should be "[GeV^2]" not "[GeV]" since it's a covariance matrix (Cov3f), representing variances and covariances of momentum components. Covariance has units that are squared relative to the base quantity.
| - edm4eic::Cov3f momentumError [GeV] // Error on the 3-momentum | |
| - edm4eic::Cov3f momentumError [GeV^2] // Error on the 3-momentum |
ruse-traveler
left a comment
ruse-traveler
left a comment
There was a problem hiding this comment.
Nice! This is much appreciated! The aligned fields and multiline declarations make me happy 😌
Overall, looks great, and I think copilot produced some good suggestions (e.g. comments that got cut off in moving the units).
Briefly, what does this PR introduce?
This PR cleans up the edm4eic yaml file a bit: