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 TDI-2 support #5

Merged
merged 11 commits into from
May 13, 2024
Merged

add TDI-2 support #5

merged 11 commits into from
May 13, 2024

Conversation

WuShichao
Copy link
Collaborator

@ahnitz @spxiwh This PR adds TDI-2 conversion to the BBHx waveform. Original BBHx's response is TDI-1.5, I'm using those equitations to convert TDI-1.5 to TDI-2.0:
image
Those are for the X/Y/Z channels, but the A/E/T channels are just linear combinations of X/Y/Z, so they still follow these.

@WuShichao WuShichao requested review from ahnitz and spxiwh April 3, 2024 16:20
@WuShichao
Copy link
Collaborator Author

@mj-will Alex wants you to review this TDI-2.0 PR. Thanks a lot!

@mj-will
Copy link
Contributor

mj-will commented Apr 29, 2024

@mj-will Alex wants you to review this TDI-2.0 PR. Thanks a lot!

Sure thing, I'm at a conference this week, so I most likely won't get to this until next week. Hope that's okay

BBHX_PhenomD.py Outdated Show resolved Hide resolved
BBHX_PhenomD.py Outdated Show resolved Hide resolved
@mj-will
Copy link
Contributor

mj-will commented Apr 29, 2024

I've added some initial comments but still need to go over things more thoroughly.

Copy link
Contributor

@mj-will mj-will left a comment

Choose a reason for hiding this comment

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

I've gone over the rest of this and it looks fine to me (except for some very minor comments). @WuShichao do you have any results produced using this that you could link to?

BBHX_PhenomD.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@WuShichao
Copy link
Collaborator Author

I've gone over the rest of this and it looks fine to me (except for some very minor comments). @WuShichao do you have any results produced using this that you could link to?

@mj-will Yes, the TDI-2 PE test is here gwastro/pycbc#4738

@WuShichao WuShichao added the enhancement New feature or request label May 13, 2024
@WuShichao WuShichao requested a review from mj-will May 13, 2024 13:16
@mj-will
Copy link
Contributor

mj-will commented May 13, 2024

@WuShichao given we've got the CI now, could you add a TDI 1.5 and 2.0 to the existing test? You should be able to use a parametrize mark to add it so the test gets run on both TDIs and reference frames.

Copy link
Contributor

@mj-will mj-will left a comment

Choose a reason for hiding this comment

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

Thanks @WuShichao, this now looks good to me!

@WuShichao WuShichao merged commit df76249 into main May 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants