search

dingo-gw / dingo

Dingo: Deep inference for gravitational-wave observations
MIT License
51 stars 16 forks source link

Changed datatype of waveforms when they fail to generate to complex #206

Closed nihargupte-ph closed 8 months ago

nihargupte-ph commented 1 year ago

Occasionally some waveforms fail to generate due to errors with the underlying LAL routines. The solution to this is to set np.nansfor these waveforms which will translate to np.nan later down in the pipeline. (For example when computing log likelihoods). However, the datatype of this nan array should be complex otherwise there will be places in the code like: https://github.com/dingo-gw/dingo/blob/main/dingo/gw/domains.py#L295 will trigger an error. This fixes the issue so that now if the log_likelihood of a invalid configuration is requested, np.nan will correctly be returned instead of a TypeError: https://github.com/dingo-gw/dingo/blob/main/dingo/gw/domains.py#L327

mpuerrer commented 1 year ago

Have you tested the change?

stephengreen commented 1 year ago

I think we only considered these possible waveform generation failures in the context of generating a waveform dataset, so the error handling was designed to work there. I hadn't thought about what happens when it fails during importance sampling, and I'm not sure we want it to get to the point of trying to manipulate the phase.

Do you know exactly when the error gets hit?

nihargupte-ph commented 1 year ago

Do you know exactly when the error gets hit?

Right, it gets hit here https://github.com/dingo-gw/dingo/blob/main/dingo/gw/domains.py#L295 which is called during a detector transform when calling GwSignal.signal. If a nan is set then the returned projected waveform will also be a nan so the likelihood would be nan. I guess these samples should not be contributing to the log_likelihood and thus should be excluded in the valid_segments when generating importance sampling. But sometimes there are waveforms which fail which would not be known a priori without generating waveforms. This would effectively modify the proposal to fit the waveform model's requirements, I guess there should be some warnings though.

We could also raise an error if there is a nan likelihood but at the moment the error message/location one gets is a bit confusing.