gwastro / BBHX-waveform-model

GNU General Public License v3.0
1 stars 3 forks source link

Adapting BBHx plugin to use higher order modes. #2

Closed ConWea closed 1 month ago

ConWea commented 10 months ago

Added two separate approximants depending on if the user would like only the 22 mode (BBHX_PhenomD) or any other mode (BBHX_PhenomHM). Also renamed the actual plugin file as to not cause confusion regarding what mode content the file gives.

WuShichao commented 9 months ago

@ConWea Thanks! I will review this PR tomorrow.

ConWea commented 9 months ago

I accidentally undid some commits and have added them back with the last two commits.

spxiwh commented 8 months ago

Can we converge on this. It is important to be able to use the HM version of the code.

@mj-will Can you work with Connor if you've also got a version of this in play?

ConWea commented 8 months ago

@spxiwh I've added some other control support to allow users to change variables in the BBHx code. For example, we've found that the length parameter can be really important (chooses points of interpolation) when recovering with HoM. Would it be easier to close this PR and open a fresh one?

I know the outstanding questions are the interpolated_fd and chirptime functions are 22 mode specific. So are there equivalent functions for other modes?

mj-will commented 8 months ago

@mj-will Can you work with Connor if you've also got a version of this in play?

@spxiwh sure, I've been using a much hackier version since I knew Connor has this version in the works. Looking at these changes, it should be trivial to switch to this version once it's ready.

ahnitz commented 7 months ago

@ConWea @mj-will What's the status of this patch?

mj-will commented 7 months ago

@ConWea @mj-will What's the status of this patch?

@ahnitz I believe the main that's left to address is this discussion about the chirp time.

From the discussion on slack, I think @WuShichao found the equations for doing this but I'm not sure if that's a change that should be made to BBHx or the pycbc function.

WuShichao commented 7 months ago

@ahnitz @mj-will @ConWea For the HM waveform (especially for SOBHB), that log_mf_min passed to get_waveform_genner is important. In order to get the correct log_mf_min, we need to take HM into account.

mj-will commented 7 months ago

@ahnitz @mj-will @ConWea For the HM waveform (especially for SOBHB), that log_mf_min passed to get_waveform_genner is important. In order to get the correct log_mf_min, we need to take HM into account.

I completely agree, my questions is whether we should update findchirp_chirptime or implement this plugin.

WuShichao commented 7 months ago

@ahnitz @mj-will @ConWea For the HM waveform (especially for SOBHB), that log_mf_min passed to get_waveform_genner is important. In order to get the correct log_mf_min, we need to take HM into account.

I completely agree, my questions is whether we should update findchirp_chirptime or implement this plugin.

Yes, but the interpolated_tf in the plugin still need some updates for that. Maybe @ahnitz can give some advice.

mj-will commented 7 months ago

@WuShichao I see what you mean. If I follow correctly, this is setting t_obs_start such that it includes the f_lower specified, so I think the correct logic would be to use the mode that gives the lowest frequency? What do you think?

ConWea commented 7 months ago

@ConWea @mj-will What's the status of this patch?

@ahnitz I am currently looking at what has been discussed regarding the chirptime function and how we can use the 22 frequencies to compute the other modes. But to be honest I am confused about it and not sure to implement it.

WuShichao commented 7 months ago

@WuShichao I see what you mean. If I follow correctly, this is setting t_obs_start such that it includes the f_lower specified, so I think the correct logic would be to use the mode that gives the lowest frequency? What do you think?

"this is setting t_obs_start such that it includes the f_lower specified" is not that right, t_obs_start set by users in the PE config file will determine the f_min by using the f_min = tf_track(t_obs_start), while "the correct logic would be to use the mode that gives the lowest frequency" is correct. @ConWea So what we need to do is update chirptime and interpolated_tf to support HMs. _f_min will be the lowest frequency at t_obs_start among all the HMs. Note that interpolated_tf just fits one mode at the moment._

WuShichao commented 4 months ago

Do we need to close this?

WuShichao commented 1 month ago

@mj-will suggests that we can close this.