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

Tides Eccentricity Behavior Fix #1318

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Tides Eccentricity Behavior Fix #1318

merged 4 commits into from
Jan 14, 2025

Conversation

veome22
Copy link
Collaborator

@veome22 veome22 commented Jan 7, 2025

Updated the behavior of tides in the KAPIL2024 model, where eccentric binaries could sometimes be spun up past synchronization due to the positive spin contribution of non-circular terms, i.e. $(l,m) \neq (2,2)$ .

See the plot below for an example of the spin evolution for a binary with the following parameters ($e=0.7$):

./COMPAS/src/COMPAS --random-seed 10 -n 1 --initial-mass-1 45.0 --initial-mass-2 18.0 --semi-major-axis 105 --eccentricity 0.7 --chemically-homogeneous-evolution-mode NONE --tides-prescription KAPIL2024 --mass-change-fraction 0.001 --radial-change-fraction 0.001

Due to the large eccentricity, the $d\Omega /dt$ term for the primary towards the beginning of CHeB is positive despite its spin exceeding the orbital frequency.
image

After the update, the spin contribution from tides reverts to the (2,2)-only contribution whenever the primary spin exceeds orbital frequency, while using the full expression when the spin is lower. This behavior better preserves synchronization throughout the binary evolution.
image

@veome22 veome22 requested a review from ilyamandel January 7, 2025 21:12
@veome22 veome22 added enhancement New feature or request severity_moderate This is a moderately severe bug urgency_moderate This is a moderately urgent issue labels Jan 7, 2025
@veome22 veome22 self-assigned this Jan 7, 2025
@veome22 veome22 requested a review from jeffriley January 8, 2025 14:15
Copy link
Collaborator

@ilyamandel ilyamandel left a comment

Choose a reason for hiding this comment

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

@veome22 , thanks for making this change.

You'll need to merge in the latest dev.

Perhaps more importantly, is there a copy-paste error from
BaseBinaryStar::CalculateDOmegaTidalDt()
to
BaseBinaryStar::CalculateDSemiMajorAxisTidalDt() ?

e2_term_spin has the value given to e2_term above, rather than the correct value for this function. But the correct value is given to e2_term, and e2_term_spin doesn't seem to be used, so maybe this has no impact? If so, please delete it, if not, please clarify.

@veome22
Copy link
Collaborator Author

veome22 commented Jan 14, 2025

@ilyamandel thank you for the review!
I have merged the dev changes so the PR should be good to go.

Perhaps more importantly, is there a copy-paste error from
BaseBinaryStar::CalculateDOmegaTidalDt()
to
BaseBinaryStar::CalculateDSemiMajorAxisTidalDt() ?

This is not a typo-- I have chosen to ignore $O(e^2)$ terms across all tidal equations based on the spin synchronization condition only. So if the star is synchronized, and tides would make it spin faster, then we want to ignore the non-(2,2) terms across $d\Omega/dt$ AND $da/dt$, ensuring that their signs are always opposite to one another. This way the angular momentum transfer from the binary to the star should remain self-consistent.

I have expanded the comments to hopefully explain this choice better. Hope this makes sense!

@veome22 veome22 requested a review from ilyamandel January 14, 2025 01:59
Copy link
Collaborator

@ilyamandel ilyamandel left a comment

Choose a reason for hiding this comment

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

Hi @veome22 ,
Thanks for the clarification -- looks good to me!

@ilyamandel ilyamandel merged commit 1676060 into dev Jan 14, 2025
3 checks passed
@ilyamandel ilyamandel deleted the tides_ecc_fix branch January 14, 2025 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request severity_moderate This is a moderately severe bug urgency_moderate This is a moderately urgent issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants