TeamCOMPAS / COMPAS

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

Fixes for issues #744 and #873 #1130

Closed jeffriley closed 4 months ago

jeffriley commented 4 months ago

@reinhold-willcox, @ilyamandel

Issue #873 : this PR changes the name of the p_NoCheck parameter to ResolveEnvelopeLoss() - weed it, we now better understand why, and the new name is consistent with other parameter names in COMPAS.

Issue #744: See issue for details, but the bottom-line is that @reinhold-willcox is right, and I have changed COMPAS to match Hurley's sse - but I was not able to generate a HeGB star to test the changes... I have tested the changes for HeMS and HeHG, but not HeGB. If either of you are able to generate an HeGB star, then you can test it or tell me how to generate the star and I'll test it. I see no reason why the code shouldn't work for HeGB, but as it is now, it is untested there.

There is a little bit of code cleanup - sorry.

ilyamandel commented 4 months ago

@jeffriley: This produces HeGB stars:

./COMPAS -n 1 --mode BSE --initial-mass-1 2.1 --initial-mass-2 2 --semi-major-axis 1

ilyamandel commented 4 months ago

(Even this one, which I suggested off the top of my head yesterday, would have produced one HeGB star ;-) ./COMPAS -n 1 --mode BSE --initial-mass-1 5 --initial-mass-2 3 --semi-major-axis 1 )

jeffriley commented 4 months ago

Huh. I wrote down 10 + 5, which didn't work... ok, I'll give this a try later.

On Thu, 16 May 2024, 11:10 am Ilya Mandel, @.***> wrote:

(Even this one, which I suggested off the top of my head yesterday, would have produced one HeGB star ;-) ./COMPAS -n 1 --mode BSE --initial-mass-1 5 --initial-mass-2 3 --semi-major-axis 1 )

— Reply to this email directly, view it on GitHub https://github.com/TeamCOMPAS/COMPAS/pull/1130#issuecomment-2113724823, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGM5XNQJPX7NL7DFTTXRAZ3ZCQBR7AVCNFSM6AAAAABHZEKSK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJTG4ZDIOBSGM . You are receiving this because you were mentioned.Message ID: @.***>

ilyamandel commented 4 months ago

Huh. I wrote down 10 + 5, which didn't work... ok, I'll give this a try later.

Then I probably said the wrong thing -- sorry!

jeffriley commented 4 months ago

Putting this back in draft. Managed to create an HeGB system - HR diagrams differ current dev vs this fix (more than expected). Will fix that problem then open this for review.

jeffriley commented 4 months ago

Ok, ready for review.

For the following binary:

./COMPAS --random-seed 0 -n 1 --mode BSE --initial-mass-1 5.6 --initial-mass-2 5.1 --semi-major-axis 2

COMPAS v02.46.04 produces the following HR diagram for the primary:

image

and the changes in the PR produce the following HR diagram:

image