IMRPhenomPv2 fix #176
IMRPhenomPv2 fix #176
Conversation
ravioli1369
requested review from
EthanMarx and
deepchatterjeeligo
and removed request for
deepchatterjeeligo and
EthanMarx
ml4gw/waveforms/cbc/phenom_d.py
Outdated
| alpha1 = self.alpha1Fit(eta, eta2, xi) | ||
| alpha2 = self.alpha2Fit(eta, eta2, xi) | ||
| alpha3 = self.alpha3Fit(eta, eta2, xi) | ||
| alpha4 = self.alpha4Fit(eta, eta2, xi) | ||
| alpha5 = self.alpha5Fit(eta, eta2, xi) | ||
|
|
||
| # merger ringdown | ||
| fRD, fDM = self.fring_fdamp(eta, eta2, chi1, chi2) | ||
| if (fRD is None) or (fDM is None): |
There was a problem hiding this comment.
For completeness, throw a RuntimeError if only one of fRD or fDM is not none.
There was a problem hiding this comment.
would be better to make the or into an and?
There was a problem hiding this comment.
Sure, but check the behavior. It's better to be explicit and to raise the error for unexpected combinations.
There was a problem hiding this comment.
yea I did that in the latest commit, could you take a look?
| @@ -409,14 +434,14 @@ def test_phenom_p(): | |||
| hc_lal_data = hc_lal.data.data[lal_mask] | |||
|
|
|||
| assert np.allclose( | |||
| 1e21 * hp_lal_data.real, 1e21 * hp_ml4gw.real.numpy(), atol=2e-3 | |||
| 1e21 * hp_lal_data.real, 1e21 * hp_ml4gw.real.numpy(), atol=1e-2 | |||
There was a problem hiding this comment.
Why was it necessary to increase the tolerance? Do we agree less closely with lalsim after these changes? Or are we testing a wider range of parameter space?
There was a problem hiding this comment.
The previous version of the waveform didn't match at all with lalsim when the tests were expanded to a larger parameter space. Now we match well in the full space, but certain parameters cause differences larger than ~6e-3, so I made it 1e-2.
* bug fix for xy spins * run pre-commit * pass fRD and fDM through to phenom_d phase and amp functions * switch to relative imports * use chirp mass to mass ratio conversion function * create tests for phenom_p * lower tolerance for phenom_p * comments and additional checks for fRD and fDM
| # ).view(-1, 1) | ||
| # interpolate at x = 1, as thats the same as f = fRD | ||
| diffRDphase = -self.interpolate( | ||
| torch.tensor([1]), x[1:-1], diffRDphase |
There was a problem hiding this comment.
The x[1:-1] and diffRDphase will have different shapes when the batch size is not one.
Probably need to let x[1:-1] follow the shape of diffRDphase so the function can correctly interpolate.
There was a problem hiding this comment.
@ravioli1369 can you look into this?
There was a problem hiding this comment.
We should also add to the tests a test that uses batched inputs
There was a problem hiding this comment.
Ok this is a good catch. They TaylorF2 and PhenomD tests had the same parameters repeated along the batch dimension. I think that is the easiest extension to the tests to ensure the plumbing is working as expected.
There was a problem hiding this comment.
Sorry for the oversight during the review.
* bug fix for xy spins * run pre-commit * pass fRD and fDM through to phenom_d phase and amp functions * switch to relative imports * use chirp mass to mass ratio conversion function * create tests for phenom_p * lower tolerance for phenom_p * comments and additional checks for fRD and fDM
No description provided.