-
Notifications
You must be signed in to change notification settings - Fork 11
Light Calorimetry: Add obj for light calo objects #158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Version v09_19_00_01, patch release for SBN2024A
update lardataobj dependency
sbnobj v10_00_08 for sbncode v10_04_07
sbnobj v10_00_10 for sbnalg v10_05_00
sbnobj v10_01_01 for sbnalg v10_06_00_03
sbnobj v10_03_00 for sbnalg v10_11_01
|
@lynnt20 could you nominate reviewers please? |
PetrilloAtWork
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class version business must be fixed.
I have left also a technical C++ comment: fell completely free to decide how to approach it.
| <class name="art::Assns<recob::Slice, sbn::LightCalo>" /> | ||
| <class name="art::Wrapper<art::Assns<recob::Slice, sbn::LightCalo>>" /> | ||
| <class name="art::Assns<sbn::LightCalo, recob::Slice, void>" /> | ||
| <class name="art::Wrapper<art::Assns<sbn::LightCalo, recob::Slice, void>>"/> | ||
|
|
||
| <class name="art::Assns<recob::OpFlash, sbn::LightCalo>" /> | ||
| <class name="art::Assns<sbn::LightCalo, recob::OpFlash, void>" /> | ||
| <class name="art::Wrapper<art::Assns<recob::OpFlash, sbn::LightCalo>>" /> | ||
| <class name="art::Wrapper<art::Assns<sbn::LightCalo, recob::OpFlash, void>>" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The void in the associations is default and not necessary, but either way it should be consistent for ease of reading. Also for ease of reading, it would be better to have them in the same order.
Finally, I was under the impression that the explicit registration of the std::pair was needed too (at least it used to be).
| <class name="art::Assns<recob::Slice, sbn::LightCalo>" /> | |
| <class name="art::Wrapper<art::Assns<recob::Slice, sbn::LightCalo>>" /> | |
| <class name="art::Assns<sbn::LightCalo, recob::Slice, void>" /> | |
| <class name="art::Wrapper<art::Assns<sbn::LightCalo, recob::Slice, void>>"/> | |
| <class name="art::Assns<recob::OpFlash, sbn::LightCalo>" /> | |
| <class name="art::Assns<sbn::LightCalo, recob::OpFlash, void>" /> | |
| <class name="art::Wrapper<art::Assns<recob::OpFlash, sbn::LightCalo>>" /> | |
| <class name="art::Wrapper<art::Assns<sbn::LightCalo, recob::OpFlash, void>>" /> | |
| <class name="std::pair<recob::Slice, sbn::LightCalo>" /> | |
| <class name="art::Assns<recob::Slice, sbn::LightCalo>" /> | |
| <class name="art::Wrapper<art::Assns<recob::Slice, sbn::LightCalo>>" /> | |
| <class name="std::pair<sbn::LightCalo, recob::Slice>" /> | |
| <class name="art::Assns<sbn::LightCalo, recob::Slice>" /> | |
| <class name="art::Wrapper<art::Assns<sbn::LightCalo, recob::Slice>>"/> | |
| <class name="std::pair<recob::OpFlash, sbn::LightCalo>" /> | |
| <class name="art::Assns<recob::OpFlash, sbn::LightCalo>" /> | |
| <class name="art::Wrapper<art::Assns<recob::OpFlash, sbn::LightCalo>>" /> | |
| <class name="std::pair<sbn::LightCalo, recob::OpFlash>" /> | |
| <class name="art::Assns<sbn::LightCalo, recob::OpFlash>" /> | |
| <class name="art::Wrapper<art::Assns<sbn::LightCalo, recob::OpFlash>>" /> |
| <class name="sbn::LightCalo"> | ||
| </class> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a checksum and a class version, so that when we change it ROOT knows how to read the old versions. Start with:
| <class name="sbn::LightCalo"> | |
| </class> | |
| <class name="sbn::LightCalo" classVersion="10" /> |
then build: it will tell you to build again. Rebuild... at this point you should see a fully successful build. Lastly, commit the new classes_def.xml to the branch.
| class LightCalo { | ||
| public: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the whole class content is public, I would declare it struct:
| class LightCalo { | |
| public: | |
| struct LightCalo { |
It is 100% equivalent, and I think clearer.
| /** | ||
| * Default constructor. | ||
| */ | ||
| LightCalo() = default; | ||
| LightCalo(double charge, double light, double energy, int bestplane) | ||
| : charge(charge) | ||
| , light(light) | ||
| , energy(energy) | ||
| , bestplane(bestplane) | ||
| {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have introduced a value constructor, and I want to make sure you are aware of the implications. I will compare yours with the same class in "plain old data" version, with no user-defined constructor (yours has two: the value constructor and the default one).
With your version, it's harder for the user to initialise the object only partially (e.g. initialise charge but not light). This is in general good, but note that partial initialization may still happen (LightCalo calo; calo.charge = 40000.0;).
With the POD version, aggregate initialisation can be used (LightCalo lc { 40000.0, 30000.0, 3.5, 2 };) that can omit the last arguments. This may or may not be desired. On the other end, POD have such guarantees (can be copied with a memcpy(), for example) that allow libraries writing them to disk to take shortcuts. I don't know whether ROOT takes advantage of any of the shortcuts.
In general I recommend POD classes, but that opens for partial initialisation.
So it's up to you and how you feel about it.
Accompanying PRs: SBNSoftware/sbndcode#878, SBNSoftware/sbncode#619, SBNSoftware/sbnanaobj#181