Add option for high frequency cutoff in pycbc_inspiral #5206
Conversation
yi-fan-wang
changed the title
|
Concerning the example test search, there are very small changes in the single detector triggers, see the binned triggers and binned histogram plots. |
add default none
pycbc/waveform/waveform.py
Outdated
| # This will override any existing approximants with the same name | ||
| # (ex. IMRPhenomXX) | ||
| cpu_td[approximant] = get_td_waveform_from_fd | ||
| if approximant not in td_apx: |
There was a problem hiding this comment.
@yi-fan-wang I don't think we can approve this. In particular, the lalsim internal conversions don't preserve phase / timing in as exact a way. This has caused several problems in the past with inconsistencies between templates and injections. If you want to be able to select between the path, I think having a way to do that would be fine, but please do not change the default here.
There was a problem hiding this comment.
@ahnitz Got it. I'll revert to remove this. (BTW I haven't understood the purpose of this. It's cherry-picked from v23_RELEASE_BRANCH and may be useful for some runs in some specific settings, though it's never in the main branch probably for the reasons you mentioned).
There was a problem hiding this comment.
@yi-fan-wang Yeah, I think addresses if you want to allow either path is probably outside the scope of this PR, so if needed consider what to do separately.
ahnitz
left a comment
ahnitz
left a comment
There was a problem hiding this comment.
@yi-fan-wang One thing broadly here, is how does this compare to setting the f_final in the template bank? That should be similar, no? If not, you may want to check to see if there are places where the high_frequency_cutoff is not currently getting supplied that it would need to be. Naively I think setting f_final is more guaranteed to affect all the right places (think all the signal consistency tests, etc, not just MF).
|
@yi-fan-wang The discussion of the purpose of the lalsimulation change (and why it musn't be included on the main branch) is had on the PR that the commit links to #4530 |
|
@ahnitz considering your question: In the FilterBank code, it looks like there is no way to specify a high frequency cutoff directly. So if the MatchedFilterControl can take f_upper, why can't FilterBank as well? (Or vice versa, why does MatchedFilterControl advertise an option if setting it doesn't consistently affect the template?) I'll also note that power_chisq_bins() has a kwarg So at present it looks like neither the bank f_final nor MatchedFilterControl can influence the actual setup of power chisq ... A bit of a mess no? |
|
I just added a commit to pass through a template end_frequency value to the power chisq bins calculation. |
4 participants
Adding options for high frequency cutoff in pybc_insiral. The motivation here is that: when downsampling, the antialising filter would only work well until ~0.85 Nyquist Frequency. This is not a problem when the gravitational wave frequency doesn't approach to that high, but may be an issue if so.
Also fix a typo in the pycbc_insiral: args.require_valid_checkpoint --> opt.require_valid_checkpoint, because args doesn't exist.
Tested with the default search example:
It turns out the ranking statistic changed a little bit