-
Notifications
You must be signed in to change notification settings - Fork 96
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
refactor(*Stack): Use local variables #1584
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughWalkthroughThe recent changes to the Changes
Possibly related PRs
Tip Announcements
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
c908f66
to
cac13ec
Compare
cac13ec
to
233e6d9
Compare
// --> Clear storage map | ||
fStoreMap.clear(); | ||
|
||
// loop over tracks created by FastSim to order the tracks | ||
for (fFSTrackIter = fFSTrackMap.begin(); fFSTrackIter != fFSTrackMap.end(); ++fFSTrackIter) { | ||
for (auto tmentry : fFSTrackMap) { |
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.
for (auto tmentry : fFSTrackMap) { | |
for (const auto [newTrack, originalTrack] : fFSTrackMap) { |
see https://en.cppreference.com/w/cpp/language/structured_binding
Perhaps you find better names even, .first
/.second
is definitely terrible for readability.
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.
TL;DR I wont do this in this PR.
To Be Honest, I have not yet udnerstood what this map is actually mapping. Yes, it's something "old" / "new". But I really don't understand it yet. So if someone else has this understanding, then I'd ask that person to go ahead and improve things!
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 comment I took the terms "new" and "original" from:
FairRoot/base/sim/FairGenericStack.h
Lines 146 to 147 in ef00300
/** FastSimulation: STL map from new track index to original track index **/ | |
std::map<Int_t, Int_t> fFSTrackMap; //! |
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.
But I agree, it does not help much.
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.
Can we resolve this and postpone for the next refactoring round?
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.
From what I remember this map was introduced when we (@karabowi and @kresan) introduced the possibility to run FastSimulation
in FairRoot. From what I remember the problem was the particle counting in the FastSimulation
mode: normally, the secondaries created in the transport engine go to the end of the stack, while in the FS
mode they were pushed just after the primary. That caused problem later on, so a map was created to track the FS
secondaries.
After the event, the FS
secondaries were to be moved to the end of the stack, and the points created by the had to be changed.
All that said. @fuhlig1 , @kresan , I think the FastSim
mode was a dead end. PANDA
is not using our implementation, they have their own. What about CBM or R3B? IMHO we could get rid of FastSim
totally.
What do you think?
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.
IMHO we could get rid of
FastSim
totally.
What do you think?
Should we move that part of the discussion to a fresh issue?
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.
TBH, I don't know how to continue with this PR: If FastSim is a "dead end", then I probably should not invent new APIs like FairGenericStack::FSTrackMapLookup
(which I currently do in this PR)?
But on the other hand, I am not feeling happy to just strip all FastSim stuff away.
If we wanted to merge this one "to simplify our current code", then I could already deprecated FairGenericStack::FSTrackMapLookup
?
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.
Maybe we can discuss the issue on our meeting next Monday. Dima is pretty sure it is not used in R3B
, cbmroot
also has no traces of FastSim
.
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.
Rethinking this, I would propose the following:
- We consider this PR independant of a possible FastSim deprecation / removal.
- Refactoring this might make the FastSim deprecation / removal even simpler, maybe
- And in a second step let's look at the FastSim stuff (please let's open an issue!)
Considering a possible deprecation / removal of these Fastsim things, I'd propose to keep the non-intuitive names for the time being.
233e6d9
to
54f11f9
Compare
54f11f9
to
2489c5d
Compare
2489c5d
to
2ab409c
Compare
2ab409c
to
1b5ae3e
Compare
... instead of member variable fFSTrackIter. Deprecate that member variable. Refactor the map lookup into FairGenericStack::FSTrackMapLookup(). Also some little related cleanups (use map.count, const).
... instead of member variable fFSTrackIter.
Deprecate that member variable.
Checklist: