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

Fix for Issue 1317 #1319

Merged
merged 2 commits into from
Jan 7, 2025
Merged

Fix for Issue 1317 #1319

merged 2 commits into from
Jan 7, 2025

Conversation

jeffriley
Copy link
Collaborator

Fix for issue 1317 - SN event not always logged to BSE log file when evolving MS merger products.

@ilyamandel I thought I had some questions about where we need to check for evolving MS merger products when there has been a stellar merger, but upon checking I think we're covered in all places, so the fix here should be all that's needed.

I am probably responsible for this problem manifesting - I have a vague recollection of adding a check for stellar merger towards the end of BaseBinaryStar::EvaluateBinary() thinking, at the time, "We don't need to do this if we've had a stellar merger". Well, we do if the stellar merger was a MS merger and we're evolving MS merger products... This is the check, now modified, at (now) line 2911 in BaseBinaryStar.cpp.

While investigating this problem I noticed that we don't always log a "TIMESTEP_COMPLETED" record to the BSE_Detailed_Output file for the very last timestep. We do log a "FINAL_STATE" record which has all the info required, but users (and particularly I think the detailed plotting code) look for the "TIMESTEP_COMPLETED" record type - so I changed the code to ensure a "TIMESTEP_COMPLETED" record is always logged, even if has the same information as the "FINAL_STATE" record.

@jeffriley jeffriley requested a review from ilyamandel January 7, 2025 23:24
@jeffriley jeffriley added bug Something isn't working severity_moderate This is a moderately severe bug urgency_low This issue is not urgent labels Jan 7, 2025
@jeffriley jeffriley self-assigned this Jan 7, 2025
@SimonStevenson
Copy link
Collaborator

Thanks for the fix Jeff, I'll check it out. Is the target branch right on this PR? Don't you want to merge into dev?

@jeffriley jeffriley merged commit 4264049 into TeamCOMPAS:issue-1317 Jan 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working severity_moderate This is a moderately severe bug urgency_low This issue is not urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supernova from main sequence merger product not recorded in BSE_Supernovae
2 participants