-
Notifications
You must be signed in to change notification settings - Fork 75
Klencki+ 2025 AM loss prescription #1425
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
Conversation
|
Note: as pointed out by Pablo Marchant, the position of the L2 point in both AM modes is fixed at the normalized distance to the L2 point to the 1/4th power, i.e While gamma is only weakly dependent on a, this is still an approximation which does not need to be made. A better approach is to implement a root solver for the polynomial whose solution gives the Lagrange point positions (see, here: https://map.gsfc.nasa.gov/ContentMedia/lagrange.pdf). This is not too complicated to implement in COMPAS, but it was not trivial for me. If we would like this functionality included, I could use some help in setting up the root solver. |
|
@reinhold-willcox I will look at this later today. Three things off the top of my head: (a) build checks are not running on github at the moment (only precommit checks - don't know why, @avivajpeyi is going to look at it). Have you successfully built COMPAS locally with your branch, with no warnings or errors? (b) if you are changing an option name or an option value (I think you are changing both), you need to deprecate the old name/value in favour of the new name/value - see the deprecation vectors towards the top of (c) is there really nothing that users (not developers) need to know about these changes? i.e. nothing to put in "What's New"? What about the new option names/values? |
jeffriley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only further request after a quick look is to check the version number - depending upon which of your PRs gets merged first.
Requesting changes for the deprecations
|
I would prefer to merge the other one first. It will take a bit of care to merge both of them because I originally had these changes on the same local branch, so I have to double check that merging them doesn't accidentally delete some code. |
|
All points addressed, and the code compiles with only the same warning as in the other PR. |
ilyamandel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Since you changed the option names, please re-create the default yaml file and add it to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good. Thanks @reinhold-willcox (pending @ilyamandel's request for a new yaml file)
|
Thanks both! |
Added KLENCKI_LINEAR angular momentum loss. This is very similar to MACLEOD_LINEAR, in that both interpolate between AM lost from the accretor (isotropic re-emission) and the L2 point. The difference is that the Macleod interpolation is linear in the separation between the two positions, while the Klencki interpolation is linear in the specific AM, gamma, directly (which goes as the square of the separation). The Klencki variant was used in Klencki+ 2025, which makes it now possible to directly compare with their inputs.
Because both require an interpolation parameter, I've renamed the parameter
--mass-transfer-jloss-macleod-linear-fraction-degento--mass-transfer-jloss-linear-fraction-degen, and similar for non-degen. As a result, this is a breaking change, although not a significant one. I am open to alternatives if we don't want to introduce a breaking change for something so minor...