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.

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 generate frequency domain waveforms at reduced frequencies #4948

Merged
merged 5 commits into from
Nov 22, 2024

Conversation

Kanchan-05
Copy link
Contributor

Standard information about the request

This is a: new feature

This change affects: pycbc_brute_bank script

This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

Motivation

Template bank generation using pycbc_brute_bank is computationally intensive for very long-duration signals, primarily due to the match calculations. The computation time can be reduced by generating frequency-domain signals at lower frequencies specifically for the match calculations.

Contents

Links to any issues or associated PRs

N/A

Testing performed

N/A

Additional notes

  • The author of this pull request confirms they will adhere to the code of conduct

@ahnitz
Copy link
Member

ahnitz commented Nov 19, 2024

The main purpose of this is for time domain signals since they must be generated at full resolution first before conversion to the frequency-domain. This adds the ability to then decimate their frequency-domain representation and use that for match calculations.

We do this already for frequency-domain waveform by just setting the buffer length to something shorter than the actual duration. However, that will now work for time domain signals.

@@ -53,6 +53,8 @@ parser.add_argument('--approximant', required=False,
parser.add_argument('--minimal-match', default=0.97, type=float)
parser.add_argument('--buffer-length', default=2, type=float,
help='size of waveform buffer in seconds')
parser.add_argument('--buffer-high-pass-length', default=None, type=float,
Copy link
Member

Choose a reason for hiding this comment

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

The name / help might be clearer. Maybe --full-resolution-buffer-length or maybe you have a better idea? Suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--full-resolution-buffer-length sounds good! If it feels too lengthy, perhaps --full-res-buffer-length could be a more concise alternative.

@spxiwh
Copy link
Contributor

spxiwh commented Nov 20, 2024

What does this gain you, in terms of computational efficiency? I would expect that the cost of generating TD waveforms (which I think is still done here at the full sample rate/length) dominates matched-filtering calculations in this application?

@Kanchan-05
Copy link
Contributor Author

@spxiwh I looked at the cProfile output (see attachment) for low-mass template bank generation, and it turns out the FFT operations for match calculations are more time-consuming than generating waveforms in the time domain.

If we go with the reduced frequency approach, we can reduce the cost of match calculations quite a bit. After that, waveform generation becomes the main bottleneck, but I think we can handle that by parallelizing it across multiple cores.

cprofile

Copy link
Member

@ahnitz ahnitz left a comment

Choose a reason for hiding this comment

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

Can you double check the code style? Codeclimate doesn't appear to be automatically running. Make sure that your code is following PEP8, etc.

Copy link
Member

@ahnitz ahnitz left a comment

Choose a reason for hiding this comment

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

@Kanchan-05 Address the one remaining comment and then feel free to merge.

@@ -53,6 +53,8 @@ parser.add_argument('--approximant', required=False,
parser.add_argument('--minimal-match', default=0.97, type=float)
parser.add_argument('--buffer-length', default=2, type=float,
help='size of waveform buffer in seconds')
parser.add_argument('--full-resolution-buffer-length', default=None, type=float,
help='size of waveform buffer in seconds')
Copy link
Member

Choose a reason for hiding this comment

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

I think this help message needs to be clear, e.g. how is this different from buffer-length? Otherwise the PR looks fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ahnitz ahnitz enabled auto-merge (squash) November 22, 2024 18:14
@ahnitz ahnitz merged commit 47d4d8d into gwastro:master Nov 22, 2024
29 checks passed
@Kanchan-05 Kanchan-05 deleted the add_reduced_freq_match_func branch November 22, 2024 20:27
@spxiwh
Copy link
Contributor

spxiwh commented Nov 22, 2024

Okay, so 3600 waveform generations and 160000 FFTs … Makes sense then!

I do note that you are using the numpy backend for FFTs, which is slow, and not using the faster Class based FFTs (there is a “do cached FFTs” function that I wrote to leverage the class based stuff in an easy way, which would probably be useful here) … This code would be an interesting test case for the CUPY backend I am playing with as well (especially if using a waveform that can be generated on a GPU).

@Kanchan-05
Copy link
Contributor Author

@spxiwh That's interesting. Could you please point me to the class that you mentioned?

@spxiwh
Copy link
Contributor

spxiwh commented Nov 24, 2024

def execute_cached_fft(invec_data, normalize_by_rate=True, ifft=False,
is the function I mean. This won't do anything with the numpy FFT backend (which is always slow), but if you install+use MKL or FFTW it will help quite a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants