-
Notifications
You must be signed in to change notification settings - Fork 75
Allow binned-cosmic-integrator to also include BNS, BHNS #1394
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
Allow binned-cosmic-integrator to also include BNS, BHNS #1394
Conversation
Hmmm seems like the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes Avi! I've checked that CosmicIntegration.py runs ok and produces the demo outputs. It doesn't really test any of the new functionality though (i.e., for BNS or WD binaries; by default it focuses on BBHs as before). I wonder if the example population there could be made to demonstrate some more of this functionality.
What was the main motivation for these changes? To keep up with #1368 ? Or is this independent of that?
It might be useful to explicitly set the dcos_included=['BBH'] option for BinaryPopulation.from_compas_h5 in the CosmicIntegration.py demo so that it is obvious that is where the selection happens. I didn't spot that immediately.
Given the additions of WD binaries in BinaryPopulation, should there be frac WDWD, frac_WDNS and frac_WDBH in generate_mock_population similar to frac_BNS etc? I guess those can be 0 by default.
If so, we will then also need to check that Pdet = 0 for WD containing binaries, since Pdet is explicitly for LIGO-type detectors that will not detect WD binaries. We should still be able to calculate cosmological formation rates for these though. I've raised a similar concern in #1368
That's all I spotted, once you've answered those I'm happy to approve
Hey Simon! Thanks for the quick review :)
There were two reasons:
Good call! I’ll make that change in the CosmicIntegration.py demo so it’s clear where the selection happens. Re: WD binaries Thanks again! I’ll push these updates shortly. |
BTW @SimonStevenson -- the 'CosmicIntegration.py' is run by the CI as well -- in case you don't want to manually rerun it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Avi!
Good to know! |
Main changes were to the 'BBHPopulation' class. This is now a
binary_population
.User can select which set of DCOs from a list of valid DCOs they'd like to be included in the binned cosmic integrator (different from the FastCosmicIntegrator, which does this in a vectorized format, but not memory efficient).