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

Change the polynomial evaluators to be specified at the object level and not at the type level #3918

Merged
merged 32 commits into from
Mar 28, 2024

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Mar 27, 2024

  1. The evaluator at the type level is obnoxious and bleeds everywhere. And really it's not a characteristic of the type.
  2. We know that we have polynomials (e.g., in elliptic integrals) where the evaluation scheme needs to be Horner for numerical stability, so passing the evaluator at construction with the coefficients is logical.
  3. While the evaluator is only used at evaluation time (doh!) it would not make sense to pick a different one for different arguments.

The evaluator is passed as a tag object (because constructors have to deduce their template parameters). If omitted, Horner is used for small degrees and Estrin for large degrees.

This is only touching polynomials. The other classes that know about evaluators will be handled in future PRs.

A modest contribution to #1922.

Benchmark Relative Change
BM_JacobiAmplitude 0.99%
BM_JacobiSNCNDN 1.9%
BM_EllipticF 11%
BM_EllipticFEΠ 15%
BM_FukushimaEllipticBDJ 16%
BM_EphemerisKSPSystem/-3 0.58%
BM_EphemerisSolarSystem<…MajorBodiesOnly>/-3 -0.11%
BM_EphemerisSolarSystem<…MinorAndMajorBodies>/-3 0%
BM_EphemerisSolarSystem<…AllBodiesAndDampedOblateness>/-3 0.86%
BM_EphemerisL4Probe<…MajorBodiesOnly,&FlowEphemerisWithAdaptiveStep>/-3 -0.86%
BM_EphemerisL4Probe<…MinorAndMajorBodies,&FlowEphemerisWithAdaptiveStep>/-3 1.6%
BM_EphemerisL4Probe<…AllBodiesAndDampedOblateness,&FlowEphemerisWithAdaptiveStep>/-3 0.59%
BM_EphemerisLEOProbe<…MajorBodiesOnly,&FlowEphemerisWithAdaptiveStep>/-3 -3.3%
BM_EphemerisLEOProbe<…MajorBodiesOnly,&FlowEphemerisWithFixedStepSLMS>/-3 0.94%
BM_EphemerisLEOProbe<…MajorBodiesOnly,&FlowEphemerisWithFixedStepSRKN>/-3 1.5%
BM_EphemerisLEOProbe<…MinorAndMajorBodies,&FlowEphemerisWithAdaptiveStep>/-3 -2.1%
BM_EphemerisLEOProbe<…MinorAndMajorBodies,&FlowEphemerisWithFixedStepSLMS>/-3 -0.33%
BM_EphemerisLEOProbe<…MinorAndMajorBodies,&FlowEphemerisWithFixedStepSRKN>/-3 -0.86%
BM_EphemerisLEOProbe<…AllBodiesAndDampedOblateness,&FlowEphemerisWithAdaptiveStep>/-3 -1.2%
BM_EphemerisLEOProbe<…AllBodiesAndDampedOblateness,&FlowEphemerisWithFixedStepSLMS>/-3 0.13%
BM_EphemerisLEOProbe<…AllBodiesAndDampedOblateness,&FlowEphemerisWithFixedStepSRKN>/-3 -1.4%
BM_EphemerisTranslunarSpaceProbe<…MajorBodiesOnly,&FlowEphemerisWithFixedStepSLMS>/-3 -4.1%
BM_EphemerisTranslunarSpaceProbe<…MinorAndMajorBodies,&FlowEphemerisWithFixedStepSLMS>/-3 -1.8%
BM_EphemerisTranslunarSpaceProbe<…AllBodiesAndDampedOblateness,&FlowEphemerisWithFixedStepSLMS>/-3 0.36%
BM_EphemerisL4Probe1Year<…MajorBodiesOnly,&FlowEphemerisWithFixedStepSLMS>/-3 1.5%
BM_EphemerisL4Probe1Year<…MinorAndMajorBodies,&FlowEphemerisWithFixedStepSLMS>/-3 -0.72%
BM_EphemerisL4Probe1Year<…AllBodiesAndDampedOblateness,&FlowEphemerisWithFixedStepSLMS>/-3 -0.13%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/-4 0.43%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/-3 -0.23%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/-2 -0.7%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/-1 -0.087%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/0 -0.12%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/1 1.4%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/2 0.81%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/3 -0.48%
BM_EphemerisFittingTolerance<&FlowEphemerisWithAdaptiveStep>/4 0.8%
BM_EphemerisStartup<&FlowEphemerisWithFixedStepSLMS>/3 0.24%
BM_EphemerisStartup<&FlowEphemerisWithFixedStepSRKN>/3 -1.7%
BM_NewhallApproximationDouble<ResultMonomialDouble,(&NewhallApproximationInMonomialBasis<double,EstrinEvaluator>)>/4 0.34%
BM_NewhallApproximationDouble<ResultMonomialDouble,(&NewhallApproximationInMonomialBasis<double,EstrinEvaluator>)>/8 0.74%
BM_NewhallApproximationDouble<ResultMonomialDouble,(&NewhallApproximationInMonomialBasis<double,EstrinEvaluator>)>/16 4.9%
BM_NewhallApproximationDisplacement<ResultMonomialDisplacement,(&NewhallApproximationInMonomialBasis<Displacement<ICRS>,EstrinEvaluator>)>/4 4.4%
BM_NewhallApproximationDisplacement<ResultMonomialDisplacement,(&NewhallApproximationInMonomialBasis<Displacement<ICRS>,EstrinEvaluator>)>/8 3.6%
BM_NewhallApproximationDisplacement<ResultMonomialDisplacement,(&NewhallApproximationInMonomialBasis<Displacement<ICRS>,EstrinEvaluator>)>/16 2.1%
BM_EvaluatePolynomialInMonomialBasisDouble<EstrinEvaluator>/4 8.5%
BM_EvaluatePolynomialInMonomialBasisDouble<EstrinEvaluator>/8 4.6%
BM_EvaluatePolynomialInMonomialBasisDouble<EstrinEvaluator>/12 -2.3%
BM_EvaluatePolynomialInMonomialBasisDouble<EstrinEvaluator>/16 -4.4%
BM_EvaluatePolynomialInMonomialBasisQuantity<EstrinEvaluator>/4 1.9%
BM_EvaluatePolynomialInMonomialBasisQuantity<EstrinEvaluator>/8 1.5%
BM_EvaluatePolynomialInMonomialBasisQuantity<EstrinEvaluator>/12 1.9%
BM_EvaluatePolynomialInMonomialBasisQuantity<EstrinEvaluator>/16 -2.1%
BM_EvaluatePolynomialInMonomialBasisVectorDouble<EstrinEvaluator>/4 0.98%
BM_EvaluatePolynomialInMonomialBasisVectorDouble<EstrinEvaluator>/8 -0.84%
BM_EvaluatePolynomialInMonomialBasisVectorDouble<EstrinEvaluator>/12 -0.085%
BM_EvaluatePolynomialInMonomialBasisVectorDouble<EstrinEvaluator>/16 -26%
BM_EvaluatePolynomialInMonomialBasisDisplacement<EstrinEvaluator>/4 1.7%
BM_EvaluatePolynomialInMonomialBasisDisplacement<EstrinEvaluator>/8 -0.51%
BM_EvaluatePolynomialInMonomialBasisDisplacement<EstrinEvaluator>/12 0.042%
BM_EvaluatePolynomialInMonomialBasisDisplacement<EstrinEvaluator>/16 -26%
BM_EvaluatePolynomialInMonomialBasisDouble<HornerEvaluator>/4 3.4%
BM_EvaluatePolynomialInMonomialBasisDouble<HornerEvaluator>/8 -1.2%
BM_EvaluatePolynomialInMonomialBasisDouble<HornerEvaluator>/12 82%
BM_EvaluatePolynomialInMonomialBasisDouble<HornerEvaluator>/16 59%
BM_EvaluatePolynomialInMonomialBasisQuantity<HornerEvaluator>/4 -39%
BM_EvaluatePolynomialInMonomialBasisQuantity<HornerEvaluator>/8 -21%
BM_EvaluatePolynomialInMonomialBasisQuantity<HornerEvaluator>/12 -2.1%
BM_EvaluatePolynomialInMonomialBasisQuantity<HornerEvaluator>/16 0.62%
BM_EvaluatePolynomialInMonomialBasisVectorDouble<HornerEvaluator>/4 4.8%
BM_EvaluatePolynomialInMonomialBasisVectorDouble<HornerEvaluator>/8 11%
BM_EvaluatePolynomialInMonomialBasisVectorDouble<HornerEvaluator>/12 -0.51%
BM_EvaluatePolynomialInMonomialBasisVectorDouble<HornerEvaluator>/16 -7.2%
BM_EvaluatePolynomialInMonomialBasisDisplacement<HornerEvaluator>/4 7.2%
BM_EvaluatePolynomialInMonomialBasisDisplacement<HornerEvaluator>/8 13%
BM_EvaluatePolynomialInMonomialBasisDisplacement<HornerEvaluator>/12 0.85%
BM_EvaluatePolynomialInMonomialBasisDisplacement<HornerEvaluator>/16 -8%

Copy link
Member

Choose a reason for hiding this comment

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

Revert this change (also the .filters).

constexpr PolynomialInMonomialBasis<V, A, PRINCIPIA_MAX(l, r), E>
friend operator-(PolynomialInMonomialBasis<V, A, l, E> const& left,
PolynomialInMonomialBasis<V, A, r, E> const& right);
Evaluator<Value_, Difference<Argument_>, degree_> const* evaluator_;
Copy link
Member

Choose a reason for hiding this comment

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

Add a setter for evaluator_; we may want to set it on the result of an operator +, or after applying a mutating operation such as +=.

@eggrobin eggrobin added the LGTM label Mar 28, 2024
@pleroy pleroy merged commit fe7f2d4 into mockingbirdnest:master Mar 28, 2024
8 checks passed
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