TeamCOMPAS / COMPAS

COMPAS rapid binary population synthesis code
http://compas.science
MIT License
65 stars 67 forks source link

Option --black-hole-kicks-mode (aka --black-hole-kicks) ignored #1219

Closed jeffriley closed 2 months ago

jeffriley commented 2 months ago

The following COMPAS runs all produce exactly the same output:

./compas --black-hole-kicks-mode zero --initial-mass-1 50 --random-seed 0
./compas --black-hole-kicks-mode reduced --initial-mass-1 50 --random-seed 0
./compas --black-hole-kicks-mode fallback --initial-mass-1 50 --random-seed 0
./compas --black-hole-kicks-mode full --initial-mass-1 50 --random-seed 0

The reason is that this call

if (thisSNevent == SN_EVENT::CCSN) {                                                        // core-collapse supernova event this timestep?
    vK = ReweightSupernovaKickByMass(vK, m_SupernovaDetails.fallbackFraction, m_Mass);      // yes - re-weight kick by mass of remnant according to user specified black hole kicks option, if relevant (default is no reweighting)
}

in BaseStar::CalculateSNKickMagnitude() is always defaulting to calling BaseStar::ReweightSupernovaKickByMass(), which just returns vK as passed. The reason BH::ReweightSupernovaKickByMass() is never called is that at the time of the call the switch of stellar type has not been done, and the star is not a remnant.

I think the simplest fix is to make BH::ReweightSupernovaKickByMass() static, and only call it (in BaseStar::CalculateSNKickMagnitude()) if the predicted stellar type of the remnant is BLACK_HOLE.

This problem was introduced in COMPAS v02.44.04 in the fix for issue #1027

urgency_high - This is a very urgent issue and should be resolved as soon as possible severity_major - This is a severe bug

Steps to reproduce the behavior: Run COMPAS as described above.

Expected behavior The different values for --black-hole-kicks-mode produce different outcomes.

Versioning

SimonStevenson commented 2 months ago

I am also wondering what happens by default when this is used in 'FALLBACK' mode with our default 'MULLERMANDEL' --remnant-mass-prescription. Does that set the value of the p_FallbackFraction? Is that parameter set to 0 by default, but the 'MULLERMANDEL' prescription is already internally accounting for fallback?

ilyamandel commented 2 months ago

MULLERMANDEL has a slightly different way of accounting for partial fallback, so I don't think this should impact that prescription.

jeffriley commented 2 months ago

@ilyamandel I confess that I haven't looked in detail, but wont BaseStar::CalculateSNKickMagnitude() call BH::ReweightSupernovaKickByMass() even for the MULLERMANDEL prescription? So vK will be reduced in addition to the treatment by the MULLERMANDEL prescription?

ilyamandel commented 2 months ago

GiantBranch::ResolveCoreCollapseSN() will set m_SupernovaDetails.fallbackFraction = 0.0; if using REMNANT_MASS_PRESCRIPTION::MULLERMANDEL .

BaseStar::DrawSNKickMagnitude() will call DrawRemnantKickMullerMandel() if using KICK_MAGNITUDE_DISTRIBUTION::MULLERMANDEL .

Then, for BLACK_HOLE_KICKS_MODE::FALLBACK, we are still applying the full kicks for the Mandel & Muller prescription. The kicks will be reduced for BLACK_HOLE_KICKS_MODE::REDUCED . It's not a combination I would recommend (the Mandel & Muller prescription is already momentum conserving by construction, so no artificial kick reduction for BHs is needed), but I am willing to let people do crazy things if they really want to. :-P

ilyamandel commented 2 months ago

I think this issue has been resolved with #1221 , and I hope my comment above addresses @SimonStevenson's question, so closing. (Of course, please feel free to reopen or create another issue if you think there's still a problem!)