gwastro / BBHX-waveform-model

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

Add support for HoM #7

Closed mj-will closed 2 months ago

mj-will commented 2 months ago

Add support for HoM.

This MR supersedes https://github.com/gwastro/BBHX-waveform-model/pull/2, since Connor is writing up and I'm going to try and get this change in for the various projects that need it.

Summary of changes

To-Do

mj-will commented 2 months ago

@WuShichao here are the plots you asked for in https://github.com/ConWea/BBHX-waveform-model/pull/2#discussion_r1568779003:

The solid line corresponds to num_interp=10 (the previous hardcoded value) and the dashed line to num_interp=100 (the proposed default)

Supermassive BBH

image

Stellar mass BBH

image

mj-will commented 2 months ago

@spxiwh I've looked at how BBHx is implemented and I don't think it automatically works out a starting frequency anywhere in the code, instead it always uses mf_min (see https://github.com/mikekatz04/BBHx/blob/master/bbhx/waveforms/phenomhm.py#L107).

So I think we can just follow the Phenom convention and set the minium frequency based on highest m mode. For reference, do you know where this is defined in LAL?

spxiwh commented 2 months ago

PyCBC does this here:

https://github.com/gwastro/pycbc/blob/master/pycbc/waveform/waveform.py#L1056

to the best of my knowledge LAL does not do this [properly], if generating f-domain waveforms in the time domain.

mj-will commented 2 months ago

The rebase was going to be a nightmare after the TDI change, so I just merged the changes from main instead.

WuShichao commented 2 months ago

Looks good and reasonable, if you can show the Q-transform of TDI waveform in the call that would be better.