chore(FairRun*): Refactor RunId Changes #1406
Conversation
fairroot/base/steer/FairRunAna.cxx
Outdated
|
|
||
| CheckRunIdChanged(0); | ||
|
|
There was a problem hiding this comment.
This is quite confusing to me!
But this is, what the old code did (at least from my reading of it).
Can someone please comment on this?
There was a problem hiding this comment.
This is a clear mistake, it should simply CheckRunIdChanged().
The FairRunAna::RunEventReco() is not being tested nor used by any experiment.
We should most likely deprecate it together with fairroot/base/event/FairEventBuilder*
There was a problem hiding this comment.
I will fix the CheckRunIdChanged in a force-psuh soon then.
It looks like PandaRoot uses some of the functions/classes you suggest to deprecate?
There was a problem hiding this comment.
From what I can see, it's only the GEM dummy implementation that was introduced by me in PandaRoot uses the code.
It did not catch up, so I guess it's not really needed.
There was a problem hiding this comment.
I am sure, you know better. :-)
Would you mind creating the PR for the deprecation? So that we can handle that independently of this PR?
467fdc5 to
7cdbbbe
Compare
7cdbbbe to
30d4c44
Compare
|
Would it make sense to change the |
|
Definitely not virtual, unless we absolutely have to (virtual comes with a performance cost; the current implementation should not change the performance in any way–except maybe for FairRunAnaProof, which hopefully goes away together with proof). I don't see a use in returning You're right: We should understand how we can unify both implementations. My current intention is to refactor the code without changing the semantics or performance. And to show opportunities (like you norticed), that can then be taken by looking more deeply. That's also, why I have put the |
30d4c44 to
1ce3acc
Compare
1ce3acc to
f96718c
Compare
fairroot/base/steer/FairRunAna.cxx
Outdated
| LOG(info) << "FairRunAna::CheckRunIdChanged() Detected changed RunID from " << fRunId << " to " << newrunid; | ||
| fRunId = newrunid; | ||
| if (fStatic) { | ||
| LOG(info) << "FairRunAna::CheckRunIdChanged: ReInit not called because initialisation is static."; |
There was a problem hiding this comment.
Please change the info to debug.
From what I remember in certain edge cases this could result in console flooded with ChangeRunId messages.
There was a problem hiding this comment.
Did so, also in FairRunOnline, where it already was info in the old code.
There was a problem hiding this comment.
Sorry, I addressed only the FairRunAna. I would prefer to keep the FairRunOnline verbosity on info, same as it was previously.
f96718c to
5d2408d
Compare
Introduce a CheckRunIdChanged in FairRunAna and FairRunOnline that handles RunId changes. Fix FairRunAna::RunEventReco: It always used runid 0 for the "new current runid". Now it just uses CheckRunIdChanged as well.
5d2408d to
5be822b
Compare
Introduce a CheckRunIdChanged in FairRunAna and FairRunOnline that handles RunId changes.
Fix FairRunAna::RunEventReco: It always used runid for the "new current runid". Now it just uses CheckRunIdChanged as well.
Inspired by #1396
Checklist: