-
Notifications
You must be signed in to change notification settings - Fork 34
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
Changes needed for YZ simulation #784
base: develop
Are you sure you want to change the base?
Conversation
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.
At this point I believe this PR is being held captive waiting for the updated WireCell Toolkit.
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.
Looks good! One note here -- we'll have to tune time_offset_{u, v, y} parameters with the new signal shape simulation going in. Not something that should hold up the PR, but a validation step we should do once we merge it in. @brucehoward-physics has a study we did to do this previously.
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.
Looks great overall! Couple questions / suggestions. Could also be ignored to merge in if there is urgency.
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.
Is the only difference here the "-shifted" jsonnet? Could you #include the base file instead and override this parameter?
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 won't hold this up, but I'm pretty sure you could clean up this code with some std.range calls: https://jsonnet.org/ref/stdlib.html.
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 another note here (which should not hold up the review). We'll need to replace these with the ICARUSDrifters that I made (#782) to lookup the electron lifetime from the SQLite file.
name: "scaler%d" %n, //%std.floor(n/45), | ||
data: params.lar { | ||
yzmap_scale_filename: 'yzmap_gain_icarus_v2_run2.json', | ||
bin_width: 10*wc.cm, |
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.
How does this handle depositions that are outside the binning of the YZ maps? I don't think I included the whole TPC when I made those.
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.
What's the difference between this config and the non-shifted one?
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.
Is this one not -refactored? Do we need it? I don't see it referenced by a fcl file.
This PR will enable the variation of the signal shapes and gain across the YZ plane as measured for Run2. The support is in here to also model the electron lifetime separately in each cryostat (or TPC, or YZ bin).
To fully use all these features the following PRs will also be required to be merged:
This simulation has been validated by @gputnam and @mrmooney (Thanks!)
This might require a patch once we reorganize all of our fcl files but the base line simulation is intended to consume "shifted" SimEnergyDeposits from the refactored LArG4.