ColmTalbot / gwpopulation

CPU/GPU agnostic gravitational-wave population inference
https://colmtalbot.github.io/gwpopulation/
MIT License
31 stars 46 forks source link

Cosmo extension for gwpopulation #88

Closed HuiTong5 closed 4 months ago

HuiTong5 commented 4 months ago

Try to add cosmology extension for gwpopulation which allows joint cosmology & population analysis, i.e., spectral siren.

The main changes are:

  1. add a cosmology-aware subclass of bilby.hyper.model.Model for cosmology related calculation
  2. have redshift models with changeable cosmology models and corresponding cosmo funcs
  3. make sure the caching step for mass distribution is correct given the situation that the source frame mass samples for spectral siren fit could vary given different cosmological parameters.
codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 94.18%. Comparing base (b7edefa) to head (5ab859e).

Files Patch % Lines
gwpopulation/models/mass.py 66.67% 2 Missing :warning:
gwpopulation/vt.py 66.67% 2 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #88 +/- ## ========================================== - Coverage 94.74% 94.18% -0.56% ========================================== Files 11 11 Lines 799 808 +9 ========================================== + Hits 757 761 +4 - Misses 42 47 +5 ``` | [Flag](https://app.codecov.io/gh/ColmTalbot/gwpopulation/pull/88/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Colm+Talbot) | Coverage Δ | | |---|---|---| | [python3.10](https://app.codecov.io/gh/ColmTalbot/gwpopulation/pull/88/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Colm+Talbot) | `94.06% <66.67%> (-0.56%)` | :arrow_down: | | [python3.11](https://app.codecov.io/gh/ColmTalbot/gwpopulation/pull/88/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Colm+Talbot) | `94.06% <66.67%> (-0.56%)` | :arrow_down: | | [python3.9](https://app.codecov.io/gh/ColmTalbot/gwpopulation/pull/88/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Colm+Talbot) | `94.05% <66.67%> (-0.56%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Colm+Talbot#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ColmTalbot commented 4 months ago

@afarah18 has some code based on this paper that we might be able to adapt to handle the cosmological conversions. It isn't as flexible as astropy, it currently just handles variable H0 and Omega_m, but I suspect it can be extended to w quite easily.

HuiTong5 commented 4 months ago

Thanks for sharing! I will take a look at the paper and Amenda's code when I have time. I have pushed some updates based on our previous discussion. I haven't done any actual calculations/testing, so there may be a lot of bugs in it. I'll do some testing soon. But I think the overall framework has improved to some extent.

ColmTalbot commented 4 months ago

The new changes generally look good! Amanda and I are working on packaging up the other cosmology implementation and I'll probably match the astropy API closely so it should be a drop in replacement that I can do later.

ColmTalbot commented 4 months ago

I made some changes to the way the modules that need the backend to change are specified, the short version is that you should now add the new module to the setup.cfg file instead of the backend.py.

I also pushed a version of the cosmology implementation that @afarah18 and I put together in https://github.com/ColmTalbot/wcosmo. It will be a fairly simple drop-in change once this has been merged and I added some similar code to what you have in this PR so that I can use it as a free-standing version in the meantime.

HuiTong5 commented 4 months ago

The new repo for cosmology implementation looks great! I noticed there content in models.py in the new repo is almost the same as the new module cosmo_models.py in this PR. What's the plan for this PR? Should I call the functions in the new repo to avoid repetitive code?

ColmTalbot commented 4 months ago

The new repo for cosmology implementation looks great! I noticed there content in models.py in the new repo is almost the same as the new module cosmo_models.py in this PR. What's the plan for this PR? Should I call the functions in the new repo to avoid repetitive code?

That repo is essentially a holding place so that I can use the code while we get the changes into gwpopulation. I should have added an attribution to you in there as it was strongly influenced by this PR, sorry! I'll do that today. My plan is:

The latter stage would involve removing models.py basically entirely.

HuiTong5 commented 4 months ago

I had a quick test on the time cost for single likelihood evaluation on the head node of pcdev3. The cosmo one costed roughly 0.53s and non cosmo one costed roughly 0.15s.

ColmTalbot commented 4 months ago

I had a quick test on the time cost for single likelihood evaluation on the head node of pcdev3. The cosmo one costed roughly 0.53s and non cosmo one costed roughly 0.15s.

Nice! Which backend is that using?

~I'm going to merge this now, so I can make an attempt at implementing the other cosmology code. Thanks @HuiTong5!~ EDIT: I just saw this is marked as draft, so I'll hold off until you remove that label.

HuiTong5 commented 4 months ago

I was using cupy

HuiTong5 commented 4 months ago

~I'm going to merge this now, so I can make an attempt at implementing the other cosmology code. Thanks @HuiTong5!~ EDIT: I just saw this is marked as draft, so I'll hold off until you remove that label.

I don't think I plan to have any other changes if you are happy with this. What label should I have? Should I have DEV or I just need to simply remove the "Draft".

ColmTalbot commented 4 months ago

Great, I removed Draft from the issue title, in future, there is a "mark as draft" button on PRs somewhere that makes it more clear.