Skip to content

Changes in run_simSND#242

Draft
AnushkaDurg6 wants to merge 6 commits intoSND-LHC:masterfrom
AnushkaDurg6:your-feature-branch
Draft

Changes in run_simSND#242
AnushkaDurg6 wants to merge 6 commits intoSND-LHC:masterfrom
AnushkaDurg6:your-feature-branch

Conversation

@AnushkaDurg6
Copy link
Collaborator

Updated run_simSND to handle data profiles for muons as option for input

@siilieva siilieva self-requested a review November 11, 2024 16:45
@siilieva
Copy link

Will have to clean up the commited changes - most added files are not needed (used only in analysis and tests).
@AnushkaDurg6 , I can help with this tomorrow.
Then, we will look at the code itself.

@olantwin
Copy link

@AnushkaDurg6 Could you explain what the purpose of this new option is?
I'm a bit surprised that a new particle gun is added for every entry in the file.

@siilieva
Copy link

@AnushkaDurg6 Could you explain what the purpose of this new option is? I'm a bit surprised that a new particle gun is added for every entry in the file.

The purpose is to have the same muon profile in MC as we do in data. So we have to sample per muon entry in the input file.
Anusha and I are working to clean up (done) and modify the code.
We will try to change the PR to draft PR

@olantwin
Copy link

Thanks for the explanation!

@siilieva siilieva marked this pull request as draft November 12, 2024 11:30
parser.add_argument("--NagoyaEmu","--useNagoyaEmulsions",dest="useNagoyaEmulsions",help="use bricks of 57 Nagoya emulsion films instead of 60 Slavich", required=False,action="store_true")
parser.add_argument("-y", dest="year", help="specify the year to generate the respective TI18 detector setup", required=False, type=int, default=2024)

parser.add_argument("--dataProfile", dest="dataProfile", help="Path to the ROOT file with Ntuple data", required=False)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though of another name: "muonDataProfile"
and then help message like
"Path to the ROOT file with muon track data"
the uses dont need to be told in the help if it is ntuple inside that file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to comment on that as well. muonDataProfile is much more self-explanatory.

Additionally, required=False is redundant, as is dest, which defaults to the name of the long option.

print("The number of events is less than or equal to the number of enteries in the data profile")
for i in range(min(ntuple.GetEntries(),options.nEvents)):
ntuple.GetEntry(i)
# Extract x, y positions and slopes from the Ntuple

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this comment be aligned to the rest?

primGen.AddGenerator(pgun)
else:
print("The number of events is more than the number of enteries in the data profile")
# Set the primary generator for the run
Copy link

@siilieva siilieva Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix the indentation of the this + the 2 lines below
these shall we executed one the loop over ntuple entries is done, but still inside the if dataProfile loop

z_plane = 250.0*u.cm
pgun = ROOT.FairBoxGenerator(13, 1) # Particle ID 13 for muons, charge is irrelevant
pgun.SetPRange(30,3500) # Set momentum
if(options.nEvents<=ntuple.GetEntries()):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for such an if statement
we need a warning that `if (options.nEvents>ntuple.GetEntries() ): print("Number of events to simulate > number of sample muon tracks. ")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can talk in person about that if once other changes are done ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! working on them now!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe my confusion with the multiple particle guns is coming in again here, but if you just want the distribution to be the same, you should be able to keep sampling.
Also, per event, is every particle gun producing a muon every event?

@olantwin
Copy link

Pro tip: force pushing makes it hard to review what changed between commits. If you use --fixup commits, you can later automatically squash them into the first commit, allowing us to easily review incremental changes, while still resulting in a single, neat commit in the end.

Thanks for the updates to to the CLI argument :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants