Skip to content
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

A range of fixes: using core radii for CE survival, fixes to #1255, #1258, #1265 #1291

Merged
merged 13 commits into from
Nov 28, 2024

Conversation

ilyamandel
Copy link
Collaborator

No description provided.

@ilyamandel ilyamandel marked this pull request as draft November 27, 2024 21:03
Copy link

github-actions bot commented Nov 27, 2024

badge

Build Successful! You can find a link to the downloadable artifact below.

Name Link
Commit f40aa4c
Logs https://github.com/TeamCOMPAS/COMPAS/actions/runs/12063933310
Download https://github.com/TeamCOMPAS/COMPAS/suites/$SUITE_ID/artifacts/$ARTIFACT_ID

@jeffriley
Copy link
Collaborator

@ilyamandel I just pushed some changes that add to your fix. There is also a question for you at the end of BaseBinaryStar::ResolveCommonEnvelopeEvent() - need to decide what to do about printing to the CE file if there was a stellar merger.

@ilyamandel
Copy link
Collaborator Author

Thanks, @jeffriley -- I hadn't meant to ask you to do my work for me, but I appreciate it. ;-)
And yes, I think we should store that there was a merger, so all good!
Now, to ensure that we indicate a merger in the Switch Log as well, I had a proposal to switch both stars to massless remnants on merger. I've now implemented that, resolving #1265... But that means we are now outputting Massless remnants in the screen output message (the CE file is fine, it's written earlier). Thoughts?

@jeffriley
Copy link
Collaborator

jeffriley commented Nov 28, 2024

No worries @ilyamandel - as soon as I read that we were still reporting HeWD I knew why, and the fix was easy (and really should already have been in - continuing evolution after a stellar merger was an oversight).

The switchlog issue. When I coded the output all I wanted to indicate was:

(a) the starting stellar type for each star
(b) the ending stellar type for each star
(c) the reason evolution stopped

In the specific case of the example in issue #1286, that worked until you put the switches to MR in :-)

By switching the stars to MR after the stellar merger, you've actually continued the evolution and changed the "ending" stellar type. But both stars becoming MR is not really physical, is it? You're just doing that so you get an entry in the switchlog? If we evolved merger products, something different would happen and we'd still get a switch, right? Just not to both MR. What I'm getting at is that I think you're doing something unphysical just so you get an entry in the switchlog, and if that's true then trying to manipulate the output to account for that is a bit of a kludge - maybe we should figure out the right way (or a better way) of getting an entry in the switchlog upon stellar merger? Maybe we should always evolve past a stellar merger and switch the stars to what they should become upon merger - but that would mean we don't output what they were just prior to the merger... Do we want to change the output so in the case of mergers we print the stellar types of the stars just prior to the merger as well as the final stellar types? Whatever we do, users have to be confident that what we output is consistent (i.e. we shouldn't change the meaning of what's output just for stellar mergers - whatever we do should be additional info). Hopefully that makes sense.

@ilyamandel ilyamandel changed the title Using core radii to determine CE survival A range of fixes: using core radii for CE survival, fixes to #1255, #1258, #1265 Nov 28, 2024
@ilyamandel ilyamandel marked this pull request as ready for review November 28, 2024 06:53
@ilyamandel
Copy link
Collaborator Author

@jeffriley -- I take your point, and will think some more about what is a sensible "short-hand" output to the screen. For now, I think what's in this PR is the best solution until we actually implement the evolution of merger products (other than MS merger products). Ready for your review!

@ilyamandel ilyamandel requested a review from jeffriley November 28, 2024 06:58
@ilyamandel ilyamandel marked this pull request as draft November 28, 2024 09:31
@ilyamandel ilyamandel marked this pull request as ready for review November 28, 2024 09:46
@jeffriley
Copy link
Collaborator

@ilyamandel what about we add a new column to the BSE switchlog file ("isMerger"), and create a function that can be called from anywhere that logs a merger record to the BSE switchlog file? It would list the from and to stellar types as the same (because no switch), and they would be whatever the stellar types were at the time of the call to log the record. The "isMerger" field would be "false" for ordinary switch logs, and "true" for the merger logs. The function to log the merger would be a no-op if the switchlog is not enabled. Thoughts? I could do this in the next day or so.

@ilyamandel
Copy link
Collaborator Author

I like that, @jeffriley ! We can take out lines 3088-3090 of the new BaseBinaryStar.cpp file -- either now or in your next PR, your call.

P.S. I am going offline now, good night. ;-)

Copy link
Collaborator

@jeffriley jeffriley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ilyamandel

@jeffriley jeffriley merged commit e3f130a into dev Nov 28, 2024
2 checks passed
@jeffriley jeffriley deleted the Output branch November 28, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants