-
Notifications
You must be signed in to change notification settings - Fork 1
69 add the ability to time shift simhits before digitizing them #73
base: trunk
Are you sure you want to change the base?
69 add the ability to time shift simhits before digitizing them #73
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.
I think this looks good, I just have some (optional) ideas on how to improve the configuration.
- Put the configuration of the time spread into functions on the python side so that the user doesn't need to remember the integers. e.g.
def uniform_spill_time_spread(self, min, max):
self.do_time_spread_per_spill = True
self.time_spread_per_spill_type = 1 # Uniform
self.time_spread_per_spill_parameters = [0.0, 1.0] # Min, Max
- Promote time spread to its own config class in python. This would avoid having a generic "parameters" variable and instead allow each of the types of spreads to have their own named variables. They would just need to share the
type
member variable (or similar) to allow the C++ side to deduce which type it is.
class TimeSpread:
def __init__(self, kind):
self.kind = kind
# instead of True/False boolean, just have separate spread class that is no spread?
class NoSpread:
def __init__(self):
super().__init__(-1)
# would do similar for constant and gaussian
class Uniform(TimeSpread):
def __init__(self, min, max):
super().__init__(1)
self.min = min
self.max = max
# setting the spread would become more readable and shorter
digi.time_spread_for_spill = Uniform(0.0, 1.0)
Then on the C++ side
const auto& time_spread_for_spill = ps.getParameter<framework::config::Parameters>("time_spread_for_spill");
if (time_spread_for_spill.getParameter<int>("kind") == 1) {
min = time_spread_for_spill.getParamter<double>("min");
}
// continue with parameter extraction...
One could even copy this class hierarchy into C++ if desired.
This sounds reasonable, I'll go ahead and test it tonight! |
fd64a90
to
b3655f5
Compare
With the impending submodule->subdirectory transition on Wednesday, I am commenting here to remind you to either merge this PR before then or close it and re-apply the changes after Wednesday to make a new PR on the subdirectory of ldmx-sw. |
I have already done the transition and am in the final stages of testing. Updating submodule changes to apply to subdirectories is not a difficult process and one that I've tested on my own branch of the ECal submodule. Please follow those instructions and re-open this PR in ldmx-sw off the new trunk. |
This resolves LDMX-Software/ldmx-sw#1343 by adding the following options to the digitizer (primarily for TB stuff)
The configurable shifts are
The producer validates that the option is one of the valid ones and that the parameter length is correct.
Here's the test configuration that I used to try the setup