-
Notifications
You must be signed in to change notification settings - Fork 116
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
Timing Correction for MuonDIS generation #510
base: master
Are you sure you want to change the base?
Conversation
882f31d
to
924ee69
Compare
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.
Very nice improvements. Generally looking good, but some nitpicks.
Once all other issues are resolved, it would be good to run clang-format and ruff over the touched code.
I would suggest fixing issues with --fixup commits, and then at the end we can do a rebase and automatically squash the fixes.
You might want to check out git-absorb to make that process a bit simpler.
Could you add some explanation to your first post, maybe with the before/after plots you sent me? |
This change would also benefit from a description in the CHANGELOG (probably an entire paragraph!) |
@maksymovchynnikov Could you please have a look and ideally test these changes? It would be good to merge these ASAP. |
@maksymovchynnikov Your monthly reminder that your feedback would be very welcome on this pull request. |
@anupama-reghunath I also noticed that there are still a lot of open comments. |
d4564d6
to
2fe2bea
Compare
Added array for softTracks for MuDIS
eb036ca
to
908e32a
Compare
For any new code, we should not use TVectorD anymore, I'll make an exception for this PR, as the muon DIS was already using it. We should consider refactoring to use STL containers. |
muonDIS/makeMuonDIS.py
Outdated
# dPartDIS.ConstructedAt(nPart).Use(part) #to be adapted later | ||
dPartDIS[nPart] = part | ||
if itrk == 1: | ||
with open(f"sigmadata_{args.nJob}.txt", "a") as fcross: |
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.
Reasoning for having an external textfile to save the DIS cross section from Pythia6:
- As is, the DIS cross section otherwise is not saved anywhere
- Consider generating N >>1 DIS events for the single muon. Since Pythia generates the cross-section value events-by-event, it converges from some meaningless value for the 1st event to the proper value for the Nth event. In the proper implementation, all the events must share the same correct cross-section. As a temporary fix,we achieve this by reading the Nth DIS cross section entry for a given muon to calculate the weight.
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.
My plan of action to avoid this external file is:
1.Directly write the cross section to the muonDis file ( which is passed as the input for muonDIS production)
2. Set weight of the scattered muon to the cross section in the simulation (atm both the incoming and scattered have the same weight from the spill).
Perhaps also set a minimum value for the number of DIS per muon to allow for convergence?
Please let me know what you think! @olantwin
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.
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.
Ok, this is now much clearer, thanks!
I agree, that this information should be directly in the sample. Is there a weight field in the event header? Otherwise, using the weight of the scattered muon seems like an OK workaround (maybe we can preserve the weight of the incoming muon from the target sim).
We do not use the cross section anywhere else, right?
Could you prepare that plot also relative to the final cross section? That allow us to estimate a good default number of events to generate.
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.
I agree, that this information should be directly in the sample. Is there a weight field in the event header?
I couldnt find one in ShipEventHeader.
Otherwise, using the weight of the scattered muon seems like an OK workaround (maybe we can preserve the weight of the incoming muon from the target sim).
At the moment I save the cross section onto the scattered muon and also refactor the incoming muon's weight to include the DIS multiplicity such that we maintain the event weight's normalisation to a spill. Previously this refactoring was explicitly done in the analysis, but the meta data (nDIS per muon) needed to be explicitly plugged in to calculate weight for any sim file.
We do not use the cross section anywhere else, right?
Only when calculating the weight of the event. ( Done during analysis)
Could you prepare that plot also relative to the final cross section? That allow us to estimate a good default number of events to generate.
For 2000 DIS events, the convergence seems to be inversely correlated with the energy of the muon.
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.
I've removed the usage of an external file, and now save the DIS cross section to MCTrack[1] instead of scattered muon(simply out of convenience for easy calculation of weights).
Removed Condor Process variable in makeMuonDIS.py
@anupama-reghunath How shall we proceed with this PR? |
Summary of the latest commits:
|
macro/run_simScript.py
Outdated
# add Carbon to PDG | ||
pdg = ROOT.TDatabasePDG.Instance() | ||
pdg.AddParticle("C", "Carbon_Nucleus", 12.0, True, 0, 6.0, "nucleus", 1000060120) |
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 would want to add this where we add the other particles to the database:
FairShip/python/shipRoot_conf.py
Line 63 in 2eb3c2d
def configure(darkphoton=None): |
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.
wasnt sure where to add this, maybe we should then also add line 445: pdg.AddParticle('W','Ion', 1.71350e+02, True, 0., 74, 'XXX', 1000741840)
if options.mudis: | ||
EnergyCut=10.*u.MeV | ||
else: | ||
EnergyCut=100.*u.MeV | ||
|
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 better as a ternary statement to avoid possible issues of variable scope.
Are you adding any new TVectorD's compared to what was used before? |
ah yes, but this is metadata -> ( number of muons within the original MuonBack event that pass the threshold), didn't know how else to save this other than in muonDis.root , incase of recalculation of weights. |
It's fine to store it in the file, but please use a |
To be specific, there is no new TVectorD but just an additional element in the existing TVectorD. I did so to avoid making new changes to the rest of the code implementation. |
Ok, that's fine then. If we're just extending an existing object I can live with it. |
Attached are the SBT response plots produced for the previous MuonDIS(involving backwards travelling muons), and with the suggested corrections with the new MuonDIS. SBThits are color coded wrt to the time of DIS. New method preserves the incoming muon's time as well as its veto response, and ensures that the event is temporally consistent.
Before:
After: