-
Notifications
You must be signed in to change notification settings - Fork 77
Defect repairs #1432
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
Defect repairs #1432
Conversation
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.
Can I suggest that we reference the Fortran code in the comments along with Hurley Eq. (23) -- e.g., "// see Hurley et al. 2000, eq 23, but the code here follows the Fortran SSE implementation". Because the previous version implemented Eq. 23 as written in the paper, the new one does not (but I trust you that it implements the Fortran code).
Do you mean the addition of the Edit: When I first made the change I added a |
|
Here's the state of affairs: Current COMPAS code: Mainsequence.cpp::CalculateGamma(), BaseStar::CalculateAnCoefficients() (GammaConstants(B_GAMMA) and GammaConstants(C_GAMMA)) BSE code zfuncs.f: So, I will push another change that adds ABS() appropriately, and clamps gamma to [0.0, gamma], in Mainsequence.cpp::CalculateGamma() |
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.
GammaConstants(C_GAMMA) = (utils::Compare(a[75], 1.0) <= 0) ? GammaConstants(B_GAMMA) : a[80]; // Hurley et al. 2000, eq 23 and discussion immediately following - <= 1.0 confirmed in BSE Fortran code
But the Hurley paper actually says
"and C=a80 unless a75=1.0 when C=B"
Equal, not \leq.
That's why I was suggesting referencing the Fortran code directly in opposition to the paper -- but I am OK with the current comment wording if you prefer that.
Not the version I'm looking at... |
@ilyamandel a couple more minor fixes related to Hurley coefficients and constants: