Conversation
|
Hi Henry, Everything looks clear and thanks for adding these infos for analysers! :)) |
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_14_02_02 SBNSoftware/sbndaq-artdaq-core@v1_10_06 SBNSoftware/sbncode#632 SBNSoftware/sbn*@SBN_SUITE_v10_14_02_03 |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for e26:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for e26:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_14_02_01 SBNSoftware/sbndaq-artdaq-core@v1_10_06 SBNSoftware/sbncode#632 SBNSoftware/sbn*@SBN_SUITE_v10_14_02_02 |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
There was a problem hiding this comment.
I left a few non-blocking suggestions:
- using the CAF standard
caf::kUninitializedIntfor initialisation - move new bools at the end (
CF-152)
Also recommended general maintenance beyond this PR:
- turning the comments on the data members into Doxygen format
- removing the virtual empty destructors (
[CF-151])
| int nhits; // the number of strip hits contributing to the space point | ||
| int tagger; // the tagger the space point is on |
There was a problem hiding this comment.
It's not your responsibility, but it would be a service to us if you could turn all these comments (not only the two new lines from your PR) into Doxygen comments, e.g.
| int nhits; // the number of strip hits contributing to the space point | |
| int tagger; // the tagger the space point is on | |
| int nhits; ///< The number of strip hits contributing to the space point. | |
| int tagger; ///< The tagger the space point is on. |
Or you can tell us "no, thank you".
There was a problem hiding this comment.
No no it's a good shout. I don't have time to do a sweep of other classes I'm responsible for but I'll do these!
|
|
||
| // TODO: Find way of adding taggers field | ||
| // std::set<SBNDCRTTagger_t> taggers; // which taggers were used to create the track | ||
| std::vector<int> taggers; // the taggers that were used to create the track |
There was a problem hiding this comment.
If you have a reference at hand (DocDB or C++ class name) add it in the comment to help people interpret e.g. what tagger 1 means).
And blah blah Doxygen format blah.
There was a problem hiding this comment.
Could you please remove the offending line above ([CF-151])? not your responsibility, it just should have never been there.
| SRCRTSpacePointMatch(); | ||
| virtual ~SRCRTSpacePointMatch() {} | ||
|
|
||
| bool matched; // whether or not a match was made |
There was a problem hiding this comment.
... and could you add Doxygen format...
| bool matched; // whether or not a match was made | ||
| SRSBNDCRTTrack track; // the track | ||
| float score; // assessment of quality of matching (depends on alg configuration) |
There was a problem hiding this comment.
Grumble Doxygen grumble ggro.
| SRCRTSpacePointMatch(); | ||
| virtual ~SRCRTSpacePointMatch() {} | ||
|
|
||
| bool matched; // whether or not a match was made |
There was a problem hiding this comment.
I suggest this data member to be moved at the end for C++ memory layout reasons ([CF-152]). If adopted, the initialiser in the constructor will also need to be moved accordingly.
| time(std::numeric_limits<float>::signaling_NaN()), | ||
| time_err(std::numeric_limits<float>::signaling_NaN()), | ||
| complete(false) | ||
| complete(false), |
There was a problem hiding this comment.
I suggest the bool data members to be moved at the end for memory layout reasons ([https://sbnsoftware.github.io/sbn/codingconv/CodingConventionsExplained.html#CF152]). Not really important (but it might change checksum, in which case the transient one should be removed from classes_def.xml).
There was a problem hiding this comment.
Even on my last day I am learning things from you Gianluca! 😅
|
❌ CI build for SBND Failed at phase ci_tests SBND on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
Co-authored-by: Gianluca Petrillo <petrillo@slac.stanford.edu>
henrylay97
left a comment
There was a problem hiding this comment.
Thanks Gianluca, very useful comments as ever!
I have responded to all comments. The checksum did need updating following the re-ordering of member variables and removal of destructors. Obviously I've only retained the most recent.
| time(std::numeric_limits<float>::signaling_NaN()), | ||
| time_err(std::numeric_limits<float>::signaling_NaN()), | ||
| complete(false) | ||
| complete(false), |
There was a problem hiding this comment.
Even on my last day I am learning things from you Gianluca! 😅
| int nhits; // the number of strip hits contributing to the space point | ||
| int tagger; // the tagger the space point is on |
There was a problem hiding this comment.
No no it's a good shout. I don't have time to do a sweep of other classes I'm responsible for but I'll do these!
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_15_00 SBNSoftware/sbndaq-artdaq-core@v1_10_06 SBNSoftware/sbncode#632 SBNSoftware/sbn*@SBN_SUITE_v10_15_00 |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for SBND Failed at phase ci_tests SBND on slf7 for e26:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
PetrilloAtWork
left a comment
There was a problem hiding this comment.
One optional change was missed, for what it's worth.
Quick checklist
git fetchand pulled the latest changes from the branch you're basing your PR against?ClassVersionby one compared to develop in classes_def.xml?Description
I am starting to put together slides and PRs to preserve work of mine that lives offline before I leave.
The SBND CRT information in the CAFs is missing a few little pieces that are useful to analyzers (and simple to add as they are already present in the reconstruction objects).
This PR adds those branches to the StandardRecord structure.
Accompanying PR: SBNSoftware/sbncode#632
It is documented in slides: https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=45715