-
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
Model of the diode trigger used for RNO-G #675
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.
Please check why this fails and take a look at my documentation comments.
trace: array of floats | ||
the signal trace | ||
window: float | ||
the integration window |
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.
In what units? Time? Samples?
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.
added :)
def get_threshold(self, trace, threshold_sigma, int_window): | ||
noise_samples = int(130 * self.sampling_rate) | ||
noise = self.get_power_int_trace(trace[10:noise_samples], int_window) | ||
threshold = threshold_sigma * np.std(noise) | ||
return threshold |
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.
Even if it is redundant, I would put 130 * units.ns * self.sampling_rate
. Is 130 a "magic number" on the radiant? If yes, say that, if not, say why 130.
This fn needs docstrings
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.
it is within the first half of our trace, so that's the number. I agree, it need explanation. :)
""" | ||
""" |
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.
Add docstrings
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.
will do!
NuRadioReco/framework/parameters.py
Outdated
@@ -58,6 +58,7 @@ class channelParameters(Enum): | |||
signal_ray_type = 16 #: type of the ray propagation path of the signal received by this channel. Options are direct, reflected and refracted | |||
signal_receiving_azimuth = 17 #: the azimuth angle of direction at which the radio signal arrived at the antenna | |||
block_offsets = 18 #: 'block' or pedestal offsets. See `NuRadioReco.modules.RNO_G.channelBlockOffsetFitter` | |||
radiant_aux_trigger_threshold = 19 #: the threshold of the radiant aux trigger |
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.
Generally, I don't like the use of this parameter since it only allows you to run one trigger. What if I want to run a simulation where I am testing the e.g. V_eff of several different trigger thresholds? I guess only the last one will be recorded
Also, you directly store (or could store) the threshold in trigger = RadiantAUXTrigger(trigger_name, threshold_sigma, int_window, number_coincidences, coinc_window, triggered_channels)
when you run the trigger
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.
Since the trigger threshold is adjusted for each trace (mimicking the threshold servo in the field) the threshold differs for each channel and each event. I don't really know how to store that. This was one idea, but maybe that's overdoing it and we dont need to save it.
threshold: float | ||
the threshold as value of sigma (a factor of snr) | ||
|
||
window: float |
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.
int_window
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.
needs units: time? bins?
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.
sorry, time :) I added it.
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.
Sorry you still didn't address the first fixing. The parameter is int_winow
instead of window
. I am looking now and this also applies to the get_power_int_trace
function
self.sampling_rate = station.get_channel(det.get_channel_ids(station.get_id())[0]).get_sampling_rate() | ||
dt = 1. / self.sampling_rate |
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.
You should get the channel ID from triggered_channels
. You don't know that the first entry in det.get_channel_ids
will be a channel that has the sampling rate that you want.
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.
Since you allow None
, I guess you have to put an if statement here like you did below
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.
True, thanks! :)
(triggered_bins, threshold) = self.get_trigger_response(channel.get_trace(), threshold_sigma, int_window) | ||
triggered_bins_channels.append(triggered_bins) | ||
|
||
if True in triggered_bins: |
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.
np.any
will be faster (optional comment/change)
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.
thanks, I change that :)
if True in triggered_bins: | ||
channels_that_passed_trigger.append(channel.get_id()) | ||
|
||
channel.set_parameter(chp.radiant_aux_trigger_threshold, threshold) |
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.
Again, I would set this in the RadiantAUXTrigger
object unless there is a good reason to put this here
With 8 days on the new job, @lpyras now of course has time to fix this, right? |
I finally cleaned this up and added units and docstrings.
Thanks! |
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.
Just two more documentation things. But thanks for the updates already.
Functionally, this looks good
threshold: float | ||
the threshold as value of sigma (a factor of snr) | ||
|
||
window: float |
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.
Sorry you still didn't address the first fixing. The parameter is int_winow
instead of window
. I am looking now and this also applies to the get_power_int_trace
function
@cg-laser any opinions on this? |
@cg-laser , @fschlueter and me discussed that we abandon this PR, since the trigger is basically a power integrated trigger with a passband filter and a set threshold. I will change to power integration trigger such that it can take a frequency range as input. In the future, trigger and signal channels will be simulated differently which solves the storage problem. Also, a fixed threshold is assumed rather than a noise riding one. |
This PR contains a model of the diode trigger behaviour by filtering the trace in the bandpass of 80-200 MHz and then taking the power of the pulse and integrating it over 11ns. Since the trigger threshold is supposed to be noise riding, a multiple of the SNR value of the trace is used as a threshold. Usually the trigger operates in a 2/3 or 2/6 coincidence mode with a time window of 60 ns.