Conversation
siilieva
left a comment
siilieva
left a comment
There was a problem hiding this comment.
lets remove the previous hit constructor versions as we add the new ones
and please use a more explicit commit message
| LOG(DEBUG) << "signal created"; | ||
| } | ||
|
|
||
| AdvMuFilterHit::AdvMuFilterHit(Int_t detID, const std::vector<const AdvMuFilterPoint*>& V) |
There was a problem hiding this comment.
I think we can safely remove the previous version of this constructor in the lines above
AdvMuFilterHit::AdvMuFilterHit(Int_t detID, const std::vector<AdvMuFilterPoint*>& V)
There was a problem hiding this comment.
Wait a sec, shouldn't this be const std::vector<AdvMuFilterPoint>&?
AFAIK the recommended way to store and read classes in TTree and RNTuple is as std::vector<T> unless not possible.
There was a problem hiding this comment.
I think we can safely remove the previous version of this constructor in the lines above
AdvMuFilterHit::AdvMuFilterHit(Int_t detID, const std::vector<AdvMuFilterPoint*>& V)
I agree, I did not remove it because i feared it would brake the current digitization
There was a problem hiding this comment.
Wait a sec, shouldn't this be
const std::vector<AdvMuFilterPoint>&? AFAIK the recommended way to store and read classes in TTree and RNTuple is asstd::vector<T>unless not possible.
That would be my ideal implementation too, as a minimal change from the current digitization i kept the pointers (also if we implement this as the only constructor the current digitization would definetly break)
There was a problem hiding this comment.
I think we can safely remove the previous version of this constructor in the lines above
AdvMuFilterHit::AdvMuFilterHit(Int_t detID, const std::vector<AdvMuFilterPoint*>& V)I agree, I did not remove it because i feared it would brake the current digitization
ok, then we can drop that removal for now, especially since we have a large PR by Nayana that relies on the old format.
We can factorize all these updates.
There was a problem hiding this comment.
Wait a sec, shouldn't this be
const std::vector<AdvMuFilterPoint>&? AFAIK the recommended way to store and read classes in TTree and RNTuple is asstd::vector<T>unless not possible.That would be my ideal implementation too, as a minimal change from the current digitization i kept the pointers (also if we implement this as the only constructor the current digitization would definetly break)
hm, then go ahead with your ideal, also advised, implementation, changing also the digitization.
Lets have all that in the PR.
| @@ -15,6 +15,7 @@ class AdvMuFilterHit : public SndlhcHit | |||
|
|
|||
| // Constructor from MuFilterPoint | |||
| AdvMuFilterHit(Int_t detID, const std::vector<AdvMuFilterPoint*>&); | |||
There was a problem hiding this comment.
AdvMuFilterHit(Int_t detID, const std::vector<AdvMuFilterPoint*>&);
this is no longer needed if we have the new version with const pointers
| LOG(DEBUG) << "signal created"; | ||
| } | ||
|
|
||
| AdvTargetHit::AdvTargetHit(Int_t detID, const std::vector<const AdvTargetPoint*>& V) |
There was a problem hiding this comment.
same as for the constructor in AdvMuFilter
| @@ -15,6 +15,7 @@ class AdvTargetHit : public SndlhcHit | |||
|
|
|||
| // Constructor from AdvTargetPoint | |||
| AdvTargetHit(Int_t detID, const std::vector<AdvTargetPoint*>&); | |||
There was a problem hiding this comment.
same as for the constructor in AdvMuFilter
olantwin
left a comment
olantwin
left a comment
There was a problem hiding this comment.
We should store std::vector<T>. The only case where I know it's not possible is for std::vector<genfit::Track*>
|
Thanks for the feedback, should i create a new folder "rntuple" with the new digitization code (and the following updated processes)? In order to leave the current digi task untouched and later at some point replace it. |
I'd work directly on the existing code. We have the previous version of the repo tagged (25.04), so we're safe to officially start migrating to RNTuple |
I agree, we should avoid having multiple versions. We have git for that. We can keep this unmerged until after Nayana's changes and then resolve any conflicts that might pop up. |
Understood, but I am not using FairTask for the digi, just plain c++ |
3 participants
Updating data structures to comply with RNTuples (and better c++).