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 #1320

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Fix for issue 1317 #1320

merged 3 commits into from
Jan 8, 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 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
@jeffriley
Copy link
Collaborator Author

Thanks @SimonStevenson - target branch is now correct :-)

Copy link
Collaborator

@SimonStevenson SimonStevenson left a comment

Choose a reason for hiding this comment

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

Thanks for the fix Jeff! I have run the code and my example system is now correctly recorded in BSE_Supernovae. All good to go from me.

@jeffriley jeffriley merged commit 603e831 into dev Jan 8, 2025
3 checks passed
@jeffriley jeffriley deleted the issue-1317 branch January 8, 2025 00:55
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