search

dingo-gw / dingo

Dingo: Deep inference for gravitational-wave observations
MIT License
55 stars 18 forks source link

Calibration marginalization #120

Closed nihargupte-ph closed 1 year ago

nihargupte-ph commented 1 year ago

Unfinished at the moment action items to complete (in order)

1). Test with importance sampling on real data GW150914 2). Write a function to calculate the calibration envelope for injection (this should be straightforward so I can do it soon) 3). Write a function that downloads the calibration envelope (see what bilby_pipe does and replicate that) 4). Unit tests. I have some local ones (graphing) but need to add to the dingo tests module.

nihargupte-ph commented 1 year ago

The next step would be to add calibration averaging, not sure if we want this feature now though.

nihargupte-ph commented 1 year ago

There still seem to be a number of comments that haven't be addressed. Mainly these pertain to

  • Use of tile versus broadcasting.
  • Some naming conventions.
  • Passing of calibration envelope to the likelihood.
  • Non-minimal changes to example is_settings.yaml. The example file is less likely to be used in the future, with dingo_is available. But it would be good to still keep it consistent with before.

Thanks for the feedback, I have fixed 1,2 and 4 let me know if it should be changed further. For passing the calibration envelope to the likelihood wouldn't this be necessary when evaluating the likelihood? It needs to know how to marginalize over the calibration draws? Or maybe I misunderstood your point

stephengreen commented 1 year ago

There still seem to be a number of comments that haven't be addressed. Mainly these pertain to

  • Use of tile versus broadcasting.
  • Some naming conventions.
  • Passing of calibration envelope to the likelihood.
  • Non-minimal changes to example is_settings.yaml. The example file is less likely to be used in the future, with dingo_is available. But it would be good to still keep it consistent with before.

Thanks for the feedback, I have fixed 1,2 and 4 let me know if it should be changed further. For passing the calibration envelope to the likelihood wouldn't this be necessary when evaluating the likelihood? It needs to know how to marginalize over the calibration draws? Or maybe I misunderstood your point

You're right. I forgot that the likelihood class inherits from the signal class, so this is setting the signal property.

It looks like there are still a couple remaining comments that need to be addressed though.

stephengreen commented 1 year ago

I fixed a couple conflicts with main.