TeamCOMPAS / COMPAS

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

Using the `--black-hole-kicks ZERO` options sets to zero not only the natal kick of BHs, but also of NSs. #1027

Closed avigna closed 6 months ago

avigna commented 11 months ago

To Reproduce Run

`./COMPAS --mode BSE --initial-mass-1 20 --semi-major-axis 10000  -n 1  --logfile-type CSV --random-seed 0 --black-hole-kicks ZERO

then get

0: Unbound binary: (Main_Sequence_>_0.7 -> Black_Hole) + (Main_Sequence_>_0.7 -> Neutron_Star)

and then compare the pertinent columns in the BSE_Supernovae.csv file.

Expected behavior This should affect BHs exclusively, and not NSs, which be reflected in the BSE_Supernovae.csv file.

Versioning

jeffriley commented 11 months ago

Despite the name of the option, and the name of the function that applies the kick (BaseStar::ApplyBlackHoleKicks()), the kicks are applied for all CCSN events, regardless of the remnant type - no check for stellar type is made (see BaseStar::CalculateSNKickMagnitude()).

If black hole kicks should be applied to black holes only (and this is not just a poorly named option and function), then the function should be implemented in the BH class - not in the base class.

So we need some guidance - is this

(a) just a case of a poorly named option and function, and "black hole kicks" should be applied to all remnants - and so we should rename the option and function, or

(b) a defect in the code, and black hole kicks should only be applied to black holes - in which case we need to fix the code, and likely move the :ApplyBlackHoleKicks() function from the BaseStar class to the BH class

@avigna thinks (b). I tend to agree. @ilyamandel, @reinhold-willcox Thoughts?

jeffriley commented 11 months ago

I never did like the way we differentiate by stellar type in BaseStar::CalculateSNKickMagnitude(), so we might look at making that code more OO as we resolve this issue.

ilyamandel commented 11 months ago

We already have the option of setting all kicks to zero by running with --kick-magnitude-distribution ZERO

I therefore agree with @avigna that --black-hole-kicks ZERO should only apply to black hole kicks.

jeffriley commented 11 months ago

@ilyamandel Just ZERO? What about the other parameters for the --black-hole-kicks option? The question is whether the option applies just to black holes, or all remnants - you're not suggesting we be selective on the value of the option are you?

ilyamandel commented 11 months ago

Indeed, I was just using that as an example, but I agree the --black-hole-kicks option should only apply to BHs.

ilyamandel commented 6 months ago

Fixed in #1109 , closing.