CHG: MReadOutAssembly class clean up#167
Open
zoglauer wants to merge 1 commit into
Open
Conversation
fhagemann
reviewed
Jun 23, 2026
fhagemann
left a comment
fhagemann
left a comment
There was a problem hiding this comment.
Looks good to me.
Mostly style guide changes but also:
- Removing
StripHitTOnlymethods - Changing
m_InDetectorfrombool[16]toarray<bool,16> - Adding a comment about a
BUG? --> If the bug still persists, keep track of it in an issue?
Comment on lines
+229
to
236
| void SetStripHitBelowThreshold_QualityFlag(const MString& Text = "") { m_StripHitBelowThreshold_QualityFlag = true; if (Text != "") m_StripHitBelowThresholdString_QualityFlag.push_back(Text); } | ||
| //! Get the Strip Hit Below Threshold quality flag | ||
| bool HasStripHitBelowThreshold_QualityFlag() const { return m_StripHitBelowThreshold_QualityFlag; } | ||
|
|
||
| //! Set the Strip Pairing quality flag | ||
| void SetStripPairing_QualityFlag(MString Text = ""){ m_StripPairing_QualityFlag = true; | ||
| if (Text != "") { m_StripPairingString_QualityFlag.push_back(Text); }} | ||
| void SetStripPairing_QualityFlag(const MString& Text = "") { m_StripPairing_QualityFlag = true; if (Text != "") m_StripPairingString_QualityFlag.push_back(Text); } | ||
| //! Get the Strip Pairing quality flag | ||
| bool HasStripPairing_QualityFlag() const { return m_StripPairing_QualityFlag; } |
There was a problem hiding this comment.
I guess this is more of a style comment, but StripPairing_QualityFlag: is the underscore intended?
|
|
||
| //! Whether event contains strip hits in given detector | ||
| bool m_InDetector[16]; | ||
| array<bool, 16> m_InDetector; |
There was a problem hiding this comment.
What's the advantage of using array here?
Comment on lines
-141
to
-144
|
|
||
| for (unsigned int DetectorID = 0; DetectorID < 16; ++DetectorID) { | ||
| m_InDetector[DetectorID] = false; | ||
| } |
There was a problem hiding this comment.
Is this the reason to choose array over bool[16]?
Comment on lines
+180
to
+185
|
|
||
| if (DetectorID >= 0 && DetectorID <= 15) { | ||
| return m_InDetector[DetectorID]; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
or just
Suggested change
| if (DetectorID >= 0 && DetectorID <= 15) { | |
| return m_InDetector[DetectorID]; | |
| } | |
| return false; | |
| return DetectorID >= 0 && DetectorID <= 15 && m_InDetector[DetectorID]; |
?
| // Remove a strip hit | ||
|
|
||
| if (i < m_StripHits.size()) { | ||
| // BUG: MHit objects retain non-owning references to this strip hit if the caller deletes it. |
There was a problem hiding this comment.
Is this bug still present or does it need to be fixed? If the latter: track as issue?
| void MReadOutAssembly::StreamEvta(ostream& S) | ||
| { | ||
| //! Stream the content in MEGAlib's evta format | ||
| // Stream the content in MEGAlib's evta format. |
There was a problem hiding this comment.
This is the first time I'm seeing a full stop at the end of a comment.
Suggested change
| // Stream the content in MEGAlib's evta format. | |
| // Stream the content in MEGAlib's `.evta` format |
| void MReadOutAssembly::StreamRoa(ostream& S, bool WithADCs, bool WithTACs, bool WithEnergies, bool WithTimings, bool WithTemperatures, bool WithFlags, bool WithOrigins, bool WithNearestNeighbors) | ||
| { | ||
| //! Stream the content in MEGAlib's evta format | ||
| // Stream the content in MEGAlib's roa format. |
There was a problem hiding this comment.
Never mind, second time ^^
Suggested change
| // Stream the content in MEGAlib's roa format. | |
| // Stream the content in MEGAlib's `.roa` format |
| void MReadOutAssembly::StreamTra(ostream& S) | ||
| { | ||
| //! Stream the content in MEGAlib's evta format | ||
| // Stream the content in MEGAlib's tra format. |
There was a problem hiding this comment.
Suggested change
| // Stream the content in MEGAlib's tra format. | |
| // Stream the content in MEGAlib's `.tra` format |
| void MReadOutAssembly::StreamBDFlags(ostream& S) | ||
| { | ||
| // Stream the BD and QA flags | ||
| // Stream the BD and QA flags for this assembly. |
There was a problem hiding this comment.
All the comments in this function end with a full-stop now. Is this intended?
This was not the case in the comments before
|
|
||
| S<<"PQ"; | ||
| // iterate through the vectorized strip pairing reduced chi squares | ||
| // Append the strip pairing reduced chi-square values. |
There was a problem hiding this comment.
If we care about maximum consistency, this is also called chi^2 in other comments. --> decide on one naming
|
And the GitHub Action tests all pass :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lot's of changes, but 99% are just code, documentation & formatting clean-up.
Unit tests should pass.