Skip to content

Conversation

@reinhold-willcox
Copy link
Collaborator

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-degen to--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...

@reinhold-willcox
Copy link
Collaborator Author

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 (aL2/a)^(1/4)

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.

@jeffriley
Copy link
Collaborator

@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 Options.h.

(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?

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.

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

@reinhold-willcox
Copy link
Collaborator Author

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.

@reinhold-willcox
Copy link
Collaborator Author

All points addressed, and the code compiles with only the same warning as in the other PR.

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.

Looks good!
Since you changed the option names, please re-create the default yaml file and add it to this PR.

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.

All good. Thanks @reinhold-willcox (pending @ilyamandel's request for a new yaml file)

@reinhold-willcox reinhold-willcox merged commit 80fee92 into dev Aug 19, 2025
3 checks passed
@reinhold-willcox reinhold-willcox deleted the klencki_AM branch August 19, 2025 13:41
@reinhold-willcox
Copy link
Collaborator Author

Thanks both!

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.

4 participants