Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Added the CBCResult class to all gw examples #899

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MattiaEmma2022
Copy link
Collaborator

Added a line in each result of CBC sampling occurrences to specify the result_class to be CBCResult

@GregoryAshton
Copy link
Collaborator

@MattiaEmma2022 it looks like the pre-commit is failing. To fix this you can

  1. Install pre-commit locally with pip install pre-commit
  2. Runpre-commit run --all-files on this branch, this will make the necessary changes
  3. Add the files and commit them

If you want to avoid this process in future, run pre-commit install.

Copy link
Collaborator

@mj-will mj-will left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this Mattia! Looks good to me, however, I think the example in supernova_example needs updating as well (it uses the GW likelihood).

@GregoryAshton
Copy link
Collaborator

Thanks for this Mattia! Looks good to me, however, I think the example in supernova_example needs updating as well (it uses the GW likelihood).

Does it make sense to use a CBCResult for a supernova analysis? I suspect that will cause issues as the CBCResult assumes the existence of certain parameters.

@mj-will
Copy link
Collaborator

mj-will commented Feb 5, 2025

Does it make sense to use a CBCResult for a supernova analysis? I suspect that will cause issues as the CBCResult assumes the existence of certain parameters.

It might be worth looking at this in more detail. I was thinking of all the interferometer, cosmology and waveform arguments which I think will still work but there may be some others that work. That said, it does use the GraviationalWaveTransient likelihood that has all the marginalizations.

From the name though, I suppose it doesn't make sense to use it here. Perhaps this is an indication we should have a base GWResult from which CBCResult inherits?

@GregoryAshton
Copy link
Collaborator

Does it make sense to use a CBCResult for a supernova analysis? I suspect that will cause issues as the CBCResult assumes the existence of certain parameters.

It might be worth looking at this in more detail. I was thinking of all the interferometer, cosmology and waveform arguments which I think will still work but there may be some others that work. That said, it does use the GraviationalWaveTransient likelihood that has all the marginalizations.

From the name though, I suppose it doesn't make sense to use it here. Perhaps this is an indication we should have a base GWResult from which CBCResult inherits?

Maybe, but I would suggest this is getting into the territory of "A foolish consistency is the hobgoblin of little minds" - i.e. I think in practise a GWResult with CBCResult subclass would be the correct way to implement it, but I don't think it is worthwhile (just my 2c).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants