Add CRT-PMT flash classification to TPC-PMT flash-matching producer#157
Add CRT-PMT flash classification to TPC-PMT flash-matching producer#157
Conversation
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
I am suggesting a change to preserve the type of the matching variable. This allows a more immediate interpretation of its value, at the cost of an added dependency¹.
[1] If adopted, please remove the (Doxygen) comment at line 19 since it will make it not possible to move MatchType to ICARUS code — it's already very hard.
|
|
||
| // NaN value to initialize data members | ||
| static constexpr float fDefault = std::numeric_limits<float>::signaling_NaN(); | ||
| static constexpr int fIntDefault = std::numeric_limits<int>::signaling_NaN(); |
There was a problem hiding this comment.
There is no such thing as an integer NaN.
| static constexpr int fIntDefault = std::numeric_limits<int>::signaling_NaN(); | |
| static constexpr int fIntDefault = std::numeric_limits<int>::lowest(); |
(but see my other comment instead)
| float flashAsymmetry { fDefault }; ///< East-West asymmetry of PEs in matched flash | ||
| geo::Point_t flashCenter { fDefault, fDefault, fDefault }; ///< Weighted mean ophit position in X,Y,Z [no meaingful X info for ophits] (cm) | ||
| geo::Vector_t flashWidth { fDefault, fDefault, fDefault }; ///< Weighted standard devitation of ophit position in X,Y,Z [no meaingful X info for ophits] (cm) | ||
| int flashClassification { fIntDefault }; ///< Flash classification according to the CRT-PMT matching |
There was a problem hiding this comment.
I would prefer the meaning of this variable not to be lost.
That means to add a dependency: #include "sbnobj/Common/CRT/CRTPMTMatching.hh" (not clear if sbnobj::Common_CRT will need to be added to the link list in CMakeLists.txt) and then declare
| int flashClassification { fIntDefault }; ///< Flash classification according to the CRT-PMT matching | |
| sbn::crt::MatchType flashClassification { sbn::crt::MatchType::unknown }; ///< Flash classification according to the CRT-PMT matching |
I am also recommending to add unknown = -1 as value of sbn::crt::MatchType.
There was a problem hiding this comment.
This makes sense -- implemented!
kjplows
moved this from Open pull requests
to Partially reviewed
in SBN software development
2 participants
This PR (based off of
v10_00_05) adds the flash classification from the CRT-PMT matching information to the best-matched flash in the TPC-PMT barycenter flash-matching producer.Associated PRs
Review
Tagging for review @francescopoppi and @PetrilloAtWork as the CRT & light gurus. Thanks!