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

Bring the notation in elliptic_integrals.{hpp,cpp} in line with the papers #2359

Merged
merged 23 commits into from
Oct 23, 2019

Conversation

eggrobin
Copy link
Member

@eggrobin eggrobin commented Oct 19, 2019

They were an odd mix between a casefolding of the papers and remnants of the FORTRAN, leading to such things as bc for “𝐵 complete” next to Bc for 𝐵𝑐, j for 𝐽 where the paper calls an index 𝑗, etc.

Also align the Biber/XeLaTeX bibliography with the one in the header file.

@@ -780,6 +780,48 @@ @TechReport{Westra2017
month = sep
}

@Article{Bulirsch1965,
author="Bulirsch, Roland",
Copy link
Member

Choose a reason for hiding this comment

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

Could we be consistent in how we name authors? As far as I can tell we generally say "R. Bulirsch".

Copy link
Member Author

Choose a reason for hiding this comment

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

In the generated PDF, we do; in the .bib file, it is best to have as much information as possible, in case we want to switch to an unabbreviated style.

Angle EllipticΠ(Angle const& φ,
double n,
double mc);
// Returns the incomplete elliptic integral of the third kind Π(φ, n|m), where
Copy link
Member

Choose a reason for hiding this comment

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

Often, n appears first in the list of parameters of Π, followed by a semicolon. But the notation varies...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fukushima uses Π(φ, n|m).

numerics/elliptic_integrals.cpp Outdated Show resolved Hide resolved
numerics/elliptic_integrals.cpp Outdated Show resolved Hide resolved
fukushima_elliptic_Js_maclaurin_n_4_4.Evaluate(n)));
// The first five coefficients Jₗ(n|m); the others are computed as needed.
// TODO(egg): J₁(n|m) = 1/3; could we make it constexpr?
double const J₁ = fukushima_elliptic_Js_maclaurin_m_0.Evaluate(m);
Copy link
Member

Choose a reason for hiding this comment

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

The change of index is confusing because J1 is for m^0 now. Could we go back to 0-based?

Copy link
Member Author

Choose a reason for hiding this comment

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

J1 is for y1 in the result; this is 0-based, there just is no constant term.
This is the convention used in the paper.

Your indexing led to -1-based indices in the coefficient of the series.

// TODO(phl): Use a binary split of [0, 1] to reduce the number of
// comparisons.
double const m = 1.0 - mc;
double const m = 1.0 - ;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's useful to swtch from mc to for this function. As I recall this is the only place where Fukushima uses this notation, and the notation is in general messy and inconsistent enough that we shouldn't add to the mess.

Copy link
Member Author

Choose a reason for hiding this comment

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

We mention K′ below (in the existing comment as well), so we use the notation from [Fuk09] here. If we start mixing notations between papers we are going to get a mess: calling K′ Kc would give its c a different semantic than in Bc.

@pleroy pleroy added the LGTM label Oct 22, 2019
@eggrobin eggrobin merged commit 33d67a6 into mockingbirdnest:master Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants