-
Notifications
You must be signed in to change notification settings - Fork 192
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
Maximal Isotropic (Horizon Penetrating) Schwarzschild coordinates for XCTS #6427
base: develop
Are you sure you want to change the base?
Maximal Isotropic (Horizon Penetrating) Schwarzschild coordinates for XCTS #6427
Conversation
Added to the Schwarzschild AnalyticSolution for the XCTS system.
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.
For the commit messages use imperative mood, i.e. I suggest
Add Maximal Isotropic (Horizon Penetrating) Schwarzschild coordinates
to the Schwarzschild AnalyticSolution for the XCTS system.
Git Commit Message Guidelines
- Use the present tense ("Add feature" not "Added feature")
- Use the imperative mood ("Move cursor to..." not "Moves cursor to...")
- Limit the first line to 72 characters or less
- If needed, a blank second line followed by a more complete description
return .25 * | ||
(2. * areal_radius + mass + | ||
sqrt(4. * square(areal_radius) + 4. * areal_radius * mass + | ||
3. * square(mass))) * | ||
pow((4. + 3. * sqrt(2.)) * (2. * areal_radius - 3. * mass) / | ||
(8. * areal_radius + 6. * mass + | ||
3. * sqrt(8. * square(areal_radius) + | ||
8. * areal_radius * mass + | ||
6. * square(mass))), | ||
1. / sqrt(2.)) - |
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.
I think here you should call maximal_isotropic_radius_from_areal
instead of reimplementing the formula.
return .25 * | ||
(2. * areal_radius + mass + | ||
sqrt(4. * square(areal_radius) + 4. * areal_radius * mass + | ||
3. * square(mass))) * | ||
pow((4. + 3. * sqrt(2.)) * (2. * areal_radius - 3. * mass) / | ||
(8. * areal_radius + 6. * mass + | ||
3. * sqrt(8. * square(areal_radius) + | ||
8. * areal_radius * mass + | ||
6. * square(mass))), | ||
1. / sqrt(2.)) - |
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.
I think here you should call maximal_isotropic_radius_from_areal
instead of reimplementing the formula.
(2. * r_areal + mass + | ||
sqrt(4. * square(r_areal) + 4. * r_areal * mass + | ||
3. * square(mass)))) * | ||
pow((8. * r_areal + 6. * mass + | ||
3. * sqrt(8. * square(r_areal) + 8. * r_areal * mass + | ||
6. * square(mass))) / | ||
((4. + 3. * sqrt(2.)) * (2. * r_areal - 3. * mass)), | ||
1. / (2. * sqrt(2.))); |
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.
I think here you should call (a templated version of) maximal_isotropic_radius_from_areal
instead of reimplementing the formula.
const DataType S = | ||
sqrt(8. * square(r_areal) + 8. * r_areal * mass + 6. * square(mass)); | ||
const double C = 4. + 3. * sqrt(2.); | ||
const DataType D = 8. * r_areal + 6. * mass + 3. * S; | ||
const DataType E = 2. * r_areal - 3. * mass; | ||
const DataType F = 2. * r_areal + mass; | ||
const DataType A = F + S / sqrt(2.); | ||
const DataType B = C * E / D; |
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.
Please add a comment with a formula for r in terms of S, C, D, E, F, A and B
const DataType S = | ||
sqrt(8. * square(r_areal) + 8. * r_areal * mass + 6. * square(mass)); | ||
const double C = 4. + 3. * sqrt(2.); | ||
const DataType D = 8. * r_areal + 6. * mass + 3. * S; | ||
const DataType E = 2. * r_areal - 3. * mass; | ||
const DataType F = 2. * r_areal + mass; | ||
const DataType A = F + S / sqrt(2.); | ||
const DataType B = C * E / D; | ||
const DataType dAdR = 2. + (4. / sqrt(2.)) * F / S; | ||
const DataType dBdR = C * (2. * D - E * (8. + 12. * F / S)) / square(D); | ||
const DataType drdR = r_iso * (dAdR / A + dBdR / (B * sqrt(2.))); |
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.
Here you repeat the calculation for drdR
. I think it would be good to factor that into a new function maximal_isotropic_radius_from_areal_deriv
.
const DataType S = | ||
sqrt(8. * square(r_areal) + 8. * r_areal * mass + 6. * square(mass)); | ||
const double C = 4. + 3. * sqrt(2.); | ||
const DataType D = 8. * r_areal + 6. * mass + 3. * S; | ||
const DataType E = 2. * r_areal - 3. * mass; | ||
const DataType F = 2. * r_areal + mass; | ||
const DataType A = F + S / sqrt(2.); | ||
const DataType B = C * E / D; | ||
const DataType dAdR = 2. + (4. / sqrt(2.)) * F / S; | ||
const DataType dBdR = C * (2. * D - E * (8. + 12. * F / S)) / square(D); | ||
const DataType drdR = r_iso * (dAdR / A + dBdR / (B * sqrt(2.))); |
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.
Factor into new function.
* | ||
* The solution remains maximally sliced, i.e. \f$K=0\f$. And the horizon in | ||
* these coordinates is at \f$r\approx 0.7793271080557972 M\f$ due to the | ||
* radial transformation from \f$R=2M\f$. |
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.
Are you sure this is correct? My understanding is that
If you agree with this, please also correct the statement above that
Added to the Schwarzschild AnalyticSolution for the XCTS system.
Proposed changes
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.