-
Notifications
You must be signed in to change notification settings - Fork 75
Cosmic integration edits to compute WDWD merger rates #1368
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
base: dev
Are you sure you want to change the base?
Cosmic integration edits to compute WDWD merger rates #1368
Conversation
Merging fast cosmic integration scripts
Hi Melanie, thanks for the contribution! Do you have a dataset (or command that produces a dataset) that works to test this change? I did a small population run with
and then ran cosmic integration with
but got
which I guess means there are no merging WDWD binaries in my sample. Perhaps we could add a check that What do you assume about Pdet for WDWD binaries? Presumably this should be 0, since WDWD binaries are not sources for ground-based GW detectors like LIGO. It wasn't clear to me whether the current code would do this. Is the science case here things like comparing to the SN 1a rate as a function of redshift? (like Figure 3/4 in Eldridge et al. 2018) We shouldn't comment out lines we are not using any more, those should be deleted. The previous behaviour is saved in the git history, so we can always go back to see what we were doing before. Should we consider enabling rate calculations for other DCO binaries (BH-WD, NS-WD etc) as well? These may be useful to someone. This could also be left to a future update. |
@avivajpeyi fyi |
Hello Simon! Thank you for reviewing everything and sorry for my delayed reply! To combat the lack of WDWDs in your stimulation I (and Lieke) would suggest adding the flag "--include-WD-binaries-as-DCO: True" and sampling with low enough M1 (i.e. of 0.9 Also, yes I agree that a check or a print statement alerting the user when there are none of the specific DCOs that they are masking for would be very helpful! I will implement that soon. Yes! We are trying to get a feel for the SNe Ia rates as a function of redshift. I have attached below examples of the WDWD merger rates with a run of I can add an error statement that warns people that the WDWD systems they are masking for are not relevant for LVK observations and making it clear that intrinsic rates are only being computed. Also, I will add the two masks for BH-WD and NS-WD systems because we were thinking of doing that soon as well! (Will also be sure to delete any comment lines – sorry about that!) |
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.
I am not a python coder, so I hope someone else (@SimonStevenson ? @jeffriley ? ) can do a proper code review. That said, here are a few queries:
"BHNS": np.logical_and(np.isin(stellar_type_1,[13,14]),np.isin(stellar_type_2,[13,14])),
Won't this flag NS-NS and BH-BH binaries too, not just NS+BH ones? Cf. original code
"BHNS": np.logical_or(np.logical_and(stellar_type_1 == 14, stellar_type_2 == 13), np.logical_and(stellar_type_1 == 13, stellar_type_2 == 14))
While adding WDWD, do you want to also add WDNS and WDBH, or at least WDCO?
type_masks["CHE_BBH"] = np.logical_and(self.CHE_mask, type_masks["BHBH"]) if types == "CHE_BBH" else np.repeat(False, len(dco_seeds))
You changed BBH to BHBH in some places but not others -- I find this mixing rather more confusing / more likely to lead to typos in future development (i.e., I don't mind if you prefer BBH or BHBH, but suggest you pick one and stick with it)
from compas_python_utils.cosmic_integration import ClassCOMPAS
import ClassCOMPAS
There are a variety of places in different files where some of the import statements were commented out and replaced. If the new imports are preferred (why? see above about my not being a python coder), just delete the old ones: commented out code lines bloat the code and make it harder to read.
-
Repeated words in text:
"will mask binaries that binaries that experience a CEE while on the HG"
The number of merging BHBHs that need a weight
I think this comment should refer to DCOs, not just BHBHs
I have not had time to check the new normalisation code, but I am concerned by the statement that it doesn't agree with a Monte Carlo calculation. I think that's a necessary test, and it would be good to understand why they disagree.
Thanks Simon and Ilya for helping reviewing this :)
I added a few lines in
Re: @ilyamandel 's points
|
Thanks, @LiekeVanSon ! If you don't find the issue with 7, let me know and I'll take a look. Can you confirm that Monte Carlo and the new analytical calculation agree when the binary fraction is fixed (not mass-dependent)? |
I can confirm the answers agree when fbin is constant (but have yet to look into the difference when fbin is varied) |
Hi @melanie-santiago26 , @LiekeVanSon ; Thanks for the changes! Thanks for the pointer about I ran a small population and everything worked as expected. For reference, I used:
and post-processed it with
I think there is still a problem with computing detection rates for WD populations. The detection rate panel in the default plot shows a non-zero value. It seems that a warning is printed in |
Hi @LiekeVanSon , @melanie-santiago26 -- it's been quite a while since the last round of comments on this. Have you had any luck with resolving these issues? |
…slow and error prone, just directly call get_compas_fraction
…called consistently with same m1_min and max etc
Hi @ilyamandel Sorry for leaving this so long! I have tried to approach this from a few angles, but I must admit I'm a bit stuck: the analytical an numerical calculations don't agree with each other when the f_bin is variable Specifically: However, when f_bin is variable (will activate when f_bin = None) the “numerical” and analytical functions disagree. ![]() I thought that the issue was that the quad in I also tried to start from scratch just using MC sampler to create a mock Universe (see notebook in zip file I must admit I didn't spend a whole lot of time de-bugging the latter MC code, because I now suspect I made a mistake in the analytical calculation after all...! Also apologies in advance for an (extra) slow response since I am transitioning to Radboud and taking a lot of offline time in August :) |
Added a bool for WDWD systems in Class COMPAS so that you can calculate the WDWD merger rates.
Added two flags (
keep_pessimistic_CEE
,keepRLOF_postCE
) to fastcosmicintegration.pyUpdated the analytical calulation of the star forming mass to allow for a varying f_binary fraction with mass now that f_binary is None, piecewise constant value for f_binary will be used reflecting Offner 2023.
Analytical derivation of the function
analytical_star_forming_mass_per_binary_using_kroupa_imf
is shown in the attached notebook: Test_fbinary_perM.zipWarning - There seems to be a small discrepancy between MC sampled average and analytical function (but I think the MC sampler might be wrong).