-
Notifications
You must be signed in to change notification settings - Fork 346
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
add ability to cache detector response to marginalize time model and us… #4806
Conversation
…e alternate sampler rate than data
@cdcapano Can I merge this? |
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.
@ahnitz Have some questions and concerns before approving. See below.
tlen = int(round(float(sample_rate) * | ||
self.whitened_data[det].duration)) | ||
flen = tlen // 2 + 1 | ||
self._whitened_data[det].resize(flen) |
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 it worth raising an error here if the sample rate is smaller than the data?
Also, this might lead to a bug... the whitened data is set when the psd is set (see here). In principle you can change the psd after the class is initialized. I don't think it's done in an inference run anywhere currently, but could be done in a notebook and/or possibly implemented later. I think it might be better to do this by redefining psd.setter
so that it calls the parent, then runs this block of code to update the whitened data. You'll need to save sample_rate
as an attribute in that case. Maybe call it something else to distinguish it from the data sample rate (likelihood_sample_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.
Ok, interesting. Really, I just need this done before we do the FFT in the likelihood to calculate the SNR time series. I'm not sure this make more sense in the psd setter, but maybe I should put it directly in the likelihood? Is the psd setter preferable over that?
Yeah, truncating should probably be an error.
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.
If you're modifying the _whitened_data
data than it would make more sense to do it in the PSD setter, to avoid the error I mentioned. But yeah, that is a round about way to just increase the sample rate of the likelihood time series. Don't you also need to zero pad the waveform in that case too? In which case, there's a bunch of 0*0 going on in the match function. What if you add this as an option to the matched_filter_core
instead? That is, it takes in an optional sample rate argument, and if provided you zero pad qtilde
just prior to doing the ifft
? In the model you would store the sample rate as an attribute and pass it to matched_filter_core
in the likelihood call. Adding that functionality to matched_filter_core
seems like it could be more broadly useful too.
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.
@cdcapano Indeed. Actually the waveform is already zeropadding to whatever the whitened data is. I guess I could just do both there (probably not too slow to just copy out the data too, so it's not replacing the original values).
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.
@cdcapano I'll make this 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.
@cdcapano I think for now I'll make this directly in the likelihood. I thought about adding to matched filter core, but I'm not sure the best strategy at the moment.
@@ -207,6 +208,7 @@ class MarginalizedTime(DistMarg, BaseGaussianNoise): | |||
def __init__(self, variable_params, | |||
data, low_frequency_cutoff, psds=None, | |||
high_frequency_cutoff=None, normalize=False, | |||
sample_rate=None, |
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 advantage of doing this instead of just increasing the sample rate of the data? Also, this might need some documentation, or maybe change the name of the argument to make it clearer what it is. People are going to think this is the data sample rate otherwise.
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.
Increasing the sample rate of the data means that you need to do data conditioning and parts of the generation at a higher sample rate. That's only useful if you actually care about the frequency band at higher frequencies. If you just need better timing resolution that would be extremely slow and memory intensive.
I think the separation is clear as the data sample rate is in the data section. This is only about the model itself. BTW, we already have this distinction for the heterodyne likelihoods when doing sky marginalization. This was just advancing the same interface / feature to the more regular model
params['dec'], | ||
params['tc']) | ||
|
||
if self.precalc_antenna_factors: |
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 is precalc_antenna_factors
set to True in a run? Is this an argument for the model? I couldn't find where it would be set.
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's already handled by the marginalization parent class. The features were implemented first for other cases, and this just extends the use to the marginalized time model.
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, but I was asking how you set that in the config file and how that's passed to the model initialization. I couldn't figure it out from looking at the code, although I didn't dig too deep.
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.
Ah, it's automatically populated if you do sky marginalization so it's the same set of settings really. These factors are calculated in any case when the sky grid is set up.
@cdcapano Is this now ok? |
@@ -3,8 +3,11 @@ | |||
name = marginalized_time | |||
low-frequency-cutoff = 30.0 | |||
|
|||
# This is the sample rate used for the model and marginalization | |||
sample_rate = 4096 | |||
|
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 updated the example to make explicit use of this feature.
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, this looks good. This seems like it would be a good thing to add to the matched filter core at some point, but this will do for now.
This implements two features into the marginalized time model.
it can now take a 'sample_rate' argument. If given, a different sample rate can be used for the marginalization than the intrinsic one of the data. Note it really should be higher than the data sample rate to make sense.
The detector response factors are now retrieved from the precalculated ones which improves the marginalization speed by ~ 50 %.