Trigger Readout Window #763
Conversation
|
Do we want to move the deprecated modules into a folder |
no, because then the usercode will fail and they won't get the deprecation message ;-) |
There was a problem hiding this comment.
This looks fine to me - as far as I can tell it really only changes the trace start times back to how they were pre-2.2.0.
I just have some nitpicky comments about multiline strings and a request to use the warnings module for DeprecationWarnings as this will make it easier to catch this stuff in the tests in the future.
I think Felix's suggestion was to move the modules to a |
I would have added to the which should avoid that user code fails. |
|
thanks for the comments! I fixed them. We still need to update the tests once we agree on the implementation |
|
What is with the |
Probably we can keep it because it is a simple module which one can use in reconstruction pipelines? |
|
yes, this module might be useful for some studies where running a trigger module would be overkill |
| def __check_sampling_rates(self, detector_sampling_rate, channel_sampling_rate): | ||
| if not self.__sampling_rate_warning_issued: # we only issue this warning once | ||
| if not np.isclose(detector_sampling_rate, channel_sampling_rate): | ||
| logger.warning( | ||
| 'triggerTimeAdjuster was called, but the channel sampling rate ' | ||
| f'({channel_sampling_rate/units.GHz:.3f} GHz) is not equal to ' | ||
| f'the target detector sampling rate ({detector_sampling_rate/units.GHz:.3f} GHz). ' | ||
| 'Traces may not have the correct trace length after resampling.' | ||
| ) | ||
| self.__sampling_rate_warning_issued = True |
There was a problem hiding this comment.
Do we need this warning at all? We ensure that we have the correct number of samples with:
# this should ensure that 1) the number of samples is even and
# 2) resampling to the detector sampling rate results in the correct number of samples
# (note that 2) can only be guaranteed if the detector sampling rate is lower than the
# current sampling rate)
number_of_samples = int(
2 * np.ceil(
detector.get_number_of_samples(station.get_id(), channel.get_id()) / 2
* sampling_rate / detector_sampling_rate
))
Maybe we can only check that detector_sampling_rate <= channel_sampling_rate?
There was a problem hiding this comment.
I think when I implemented this I thought about it sufficiently to realize that if sampling_rate < detector_sampling_rate we cannot guarantee the correct number of samples (with the way resampling is currently implemented); I am not actually sure if the correct number of samples is guaranteed in all cases if sampling_rate > detector_sampling_rate; my feeling was that this is only true in most cases which is why I left the warning as it is. If you can work out the maths and figure out that this is wrong and we don't need the warning we can change this : )
|
I'm a bit confused, and think potentially more changes are needed. The triggerTimeAdjuster (now channelReadoutWindowCutter) is called in line 1522; later non-trigger channels are added still, with (in the new convention) wrong trace start times; I am also not 100% sure that the slightly hacky looking adjustments to pre_trigger_time by making a deep copy of the sim channels were correct, but in any case I guess with the new convention for trace_start_time they aren't correct now. Is there any reason why we do not just call the triggerTimeAdjuster (now channelReadoutWindowCutter) at the end of the simulation, after downsampling to the detector sampling rate has already occurred? I feel like this would be much less error-prone. I guess there will be a small performance impact because we have to generate more noise, but probably this only makes a small difference in the grand scheme of things? |
|
Also also (seeing as I looked at this today) - please update the documentation page |
the trace_start_time will include the (channel-specific) trigger time delays at the end of a NuRadioMC simulation.
For a clean implementation and to avoid bugs in old code, a new module channelReadoutWindowCutter was written. The previous
triggerTimeAdjustermodule will be deprecated in a future release.