Skip to content
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.

Sign up for GitHub

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 a feature to load "template_duration" from a template bank #4921

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add a feature to load "template_duration" from a template bank #4921

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51fd57d
add duration
yi-fan-wang Aug 17, 2024
899fb22
the last commit doesn't work, change the logic
yi-fan-wang Aug 17, 2024
637facb
Merge branch 'gwastro:master' into add_duration
yi-fan-wang Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions pycbc/waveform/bank.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -824,13 +824,21 @@ def __getitem__(self, index):
# If available, record the total duration (which may
# include ringdown) and the duration up to merger since they will be
# erased by the type conversion below.
ttotal = template_duration = None
if hasattr(htilde, 'length_in_time'):
ttotal = htilde.length_in_time
if hasattr(htilde, 'chirp_length'):
else:
ttotal = None
# First try to load template_duration from get_waveform_filter,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the template bank file have priority if it is there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As currently written - as a fallback - this is a simple addition. With Alex's proposal - as a change to the default behaviour - this becomes a potential problem. The reason is that some template bank codes populate a "template duration", which in many cases is different from what the search code is going to use (often around choice of f_lower). For safety, where PyCBC can compute the duration, it should do so ... I guess the alternative is that the bank2hdf code also becomes a "check the bank validity" code, and should check/update these values if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spxiwh That's a good point. I think we should at least have a logging message though so that one can verify what actually happened. I know at some point this is going to cause someone a debugging headache.

# if not exists, check if the template bank file contains template_duration,
# if none of them exists, return None
if hasattr(htilde, 'chirp_length') and htilde.chirp_length is not None:
template_duration = htilde.chirp_length

self.table[index].template_duration = template_duration
self.table[index].template_duration = template_duration
elif self.table[index].template_duration > 0:
template_duration = self.table[index].template_duration
else:
template_duration = None
self.table[index].template_duration = None

htilde = htilde.astype(self.dtype)
htilde.f_lower = f_low
Expand Down
Loading