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 an estimator for end frequency of the any custom waveform model #4911

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

Kanchan-05
Copy link
Contributor

Standard information about the request

This is a new feature added for estimating the end frequency of any custom waveform model

This feature will affect plugin.py for waveform

This change affects: the offline search, the live search, inference, PyGRB

This change changes: documentation, scientific output

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

This change will: break current functionality, require a new release

Motivation

Custom waveform approximants may or may not have a frequency-domain version. To make any time-domain waveform useful for searches or parameter estimation, get_fd_waveform_from_td is used. For an accurate time-to-frequency domain transformation, the sampling rate must be appropriately adjusted. This adjustment depends on both the duration of the signal in the time domain and the end frequency of the waveform. The signal duration and end frequency can vary depending on the source parameters and waveform approximants.

While PyCBC already includes a function to estimate the signal duration, it does not yet have a way to estimate the end frequency. That is why adding an end frequency estimator is important.

Contents

Links to any issues or associated PRs

Testing performed

Additional notes

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

"""
from pycbc.waveform.waveform import _filter_ends
if approximant in _filter_ends:
raise RuntimeError("Can't load length estimator {}, the name is"
Copy link
Member

Choose a reason for hiding this comment

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

The message here should be updated.

@ahnitz
Copy link
Member

ahnitz commented Oct 16, 2024

@Kanchan-05 You need to rebase this PR from master (don't do a merge commit)

@ahnitz
Copy link
Member

ahnitz commented Oct 16, 2024

@Kanchan-05 You seem to be committing a bunch of files unrelated to your PR. Also, you need to rebase from gwastro's master, not your local one if that's what you did.

@ahnitz
Copy link
Member

ahnitz commented Oct 16, 2024

@Kanchan-05 Address the codeclimate issues, otherwise, this looks reasonable.

@ahnitz ahnitz enabled auto-merge (squash) October 16, 2024 21:07
@ahnitz ahnitz merged commit 72e4224 into gwastro:master Oct 16, 2024
30 checks passed
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.

2 participants