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

The new version of BBHx has changed how shift_t_limits work #21

Open
spxiwh opened this issue Nov 11, 2024 · 5 comments
Open

The new version of BBHx has changed how shift_t_limits work #21

spxiwh opened this issue Nov 11, 2024 · 5 comments
Assignees

Comments

@spxiwh
Copy link
Contributor

spxiwh commented Nov 11, 2024

In the version of BBHx we have coded against we set the kwarg shift_t_limits to false. BBHX says

shift_t_limits (bool, optional) – If False, t_obs_start and t_obs_end are relative to t_ref counting backwards in time. If True, those quantities are relative to 
. (Default: False)

However, now this is deprecated, and is ignored ... It is always set to True. This then breaks the code as the times are nonsensical. I guess we can ignore t_obs_end if we are generating "full" waveforms, but t_obs_start is not always trivial (and may cause waveforms to "wrap around"). Not really sure what behaviour we want here.

@ahnitz
Copy link
Member

ahnitz commented Nov 11, 2024

@WuShichao Can you make sure our tests are pinning a BBHx version while this is sorted out?

@spxiwh
Copy link
Contributor Author

spxiwh commented Nov 11, 2024

They are ... well they're pinning a fork that isn't going to change https://github.com/gwastro/pycbc/blob/master/tox.ini#L74

@ahnitz
Copy link
Member

ahnitz commented Nov 11, 2024

@spxiwh I think the first step is to update the code to replicate the current references. It looks like one could just set t_ref to tc and appropriately set the other options to get essentially the same result. This may require some testing.

@spxiwh
Copy link
Contributor Author

spxiwh commented Nov 11, 2024

I think that we already have t_ref=tc (I think this is the default), and t_ref_end is not used at all. So the only issue I see is with translating t_ref_start ... I guess this just becomes t_c - t_ref_start_old.

@WuShichao
Copy link
Collaborator

@spxiwh Thanks for reporting this issue. Actually, t_obs_end is helpful for stellar-mass BBH; if a BBH needs decades to merge after it shows up in the LISA band, this will allow BBHx to generate early inspiral parts. Otherwise, there will be an out-of-memory issue.

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

No branches or pull requests

3 participants