TeamCOMPAS / COMPAS

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

Faulty --neutrino-mass-loss-BH-formation #1000

Closed avigna closed 6 months ago

avigna commented 1 year ago

It seems to me that the --neutrino-mass-loss-BH-formation option is not working properly. This was first noted by Jakob Stegmann. Following a quick check on his data I agreed with his take. I then proceeded to test this using the SSE mode of COMPAS by comparing the output of the following lines:

./COMPAS --mode SSE --metallicity 0.0142 --initial-mass-max 100.0 --initial-mass 80 --detailed-output --logfile-type 'CSV' -n 1

./COMPAS --mode SSE --metallicity 0.0142 --initial-mass-max 100.0 --initial-mass 80 --detailed-output --logfile-type 'CSV' -n 1 --neutrino-mass-loss-BH-formation 'FIXED_MASS' --neutrino-mass-loss-BH-formation-value 2.0

The output is identical. I would expect the black hole masses to be different.

jeffriley commented 1 year ago

@avigna, I believe

 --neutrino-mass-loss-BH-formation 'FIXED_MASS' --neutrino-mass-loss-BH-formation-value 2.0

will only have an effect if --remnant-mass-prescription is FRYER2012 or FRYER2022. The default is MULLERMANDEL

If I run COMPAS with

./COMPAS --mode SSE --metallicity 0.0142 --initial-mass-max 100.0 --initial-mass 80 --detailed-output --logfile-type CSV -n 1 --neutrino-mass-loss-BH-formation 'FIXED_MASS' --neutrino-mass-loss-BH-formation-value 2.0 --remnant-mass-prescription FRYER2022

I get different values for the BH mass. Is this not correct behaviour?

(fyi - note CSV doesn't need to be quoted for --logfile-type CSV - no harm if you do, just not required)

EDIT: I think

 --neutrino-mass-loss-BH-formation 'FIXED_MASS' --neutrino-mass-loss-BH-formation-value 2.0

would also have an effect if there was a PPISN - but there wasn't here.

avigna commented 1 year ago

@jeffriley , good catch! I actually think that this should be independent of the remnant mass prescription... but maybe that is more a feature than a bug. Uncertain wether or not we should check what is going on for PPISN and if we should adapt the code to make it more general.

Let's leave this issue open for a few days or a couple of weeks, in case anyone wants to further comment; if not, we can close it. We can always re-open it later if needed and now we have this issue documented.

jeffriley commented 1 year ago

@ilyamandel @FloorBroekgaarden @reinhold-willcox

reinhold-willcox commented 1 year ago

Agreed with @avigna , this is a feature that we probably want to fix. I can't really address this in a short timescale, but I can try to get to it in the next month or so if no one else wants it.

ilyamandel commented 1 year ago

Not sure it makes sense within that prescription: Mandel & Mueller (2020) explicitly talks about neutrino mass losses and argues that they are already effectively included and there is no need to include them further. Maybe we could just update the documentation instead?

On 18 Oct 2023, at 10:15, Reinhold Willcox @.***> wrote:

Agreed with @avigna , this is a feature that we probably want to fix. I can't really address this in a short timescale, but I can try to get to it in the next month or so if no one else wants it. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

ilyamandel commented 6 months ago

Addressed with a documentation change in #1109