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

decouple basis from storage type #517

Merged
merged 65 commits into from
Aug 15, 2023
Merged

Conversation

jverzani
Copy link
Member

@jverzani jverzani commented Aug 4, 2023

(This was the second part of #513.)

This should be non-breaking save for the removal of a few deprecations and allowing trailing zeros in ImmutablePolynomial for inference reasons, but as there was significant code-churn and there is a chance it does break some code out there, the version number will be rolled over to v4.0.0.

Prior, the polynomial type implied both a basis and a backend storage type. E.g., ImmutablePolynomial implied the standard basis and an ntuple for storage.

This PR breaks this up so the container types are re-usable and the basis is explicit -- the equivalent being ImmutableDensePolynomial{StandardBasis}. This is aliased to ImmutablePolynomial, similarly for the other standard basis polynomial types, so as to keep continuity.

The main types are

  • MutableDenseUnivariatePolynomial backed by a vector (used by Polynomial)
  • MutableDensePolynomial backed by a vector. This type has an offset, so it will allow LaurentPolynomial to be MutableDenseLaurentPolynomial{StandardBasis}.
  • MutableDenseViewPolynomial non copying when possible (used by PnPolynomial)
  • ImmutableDensePolynomial backed by an ntuple. For better type stability, trailing zeros may be present, unlike for the mutable dense polynomial type (used by ImmutablePolynomial)
  • MutableSparsePolynomial backed by a dictionary (used by SparsePolynomial)

Some simple benchmarking shows some performance issues have been improved.

 Row │ poly_eval  scalar_add  scalar_mul  scalar_div  poly_add  poly_mul  mixed_var_add  mixed_var_mul  poly_diff  poly_int  type
─────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   n │ 34.0ns     27.5ns      26.9ns      26.9ns      24.2ns    98.3ns    27.0ns         33.5ns         36.1ns     45.0ns    ImmutablePolynomial
   o │ 32.5ns     30.2ns      33.7ns      1.3μs       27.5ns    100.2ns   25.4ns         28.9ns         1.6μs      1.6μs     ImmutablePolynomial
   n │ 10.5ns     48.4ns      54.0ns      55.1ns      57.2ns    218.1ns   72.6ns         79.4ns         60.3ns     61.8ns    Polynomial
   o │ 13.7ns     46.1ns      44.0ns      89.4ns      63.7ns    255.2ns   72.6ns         71.2ns         353.2ns    392.4ns   Polynomial   
   n │ 10.5ns     64.0ns      59.4ns      60.1ns      74.1ns    225.2ns   91.2ns         87.3ns         128.1ns    139.8ns   LaurentPolynomial
   o │ 11.0ns     76.1ns      113.2ns     115.5ns     84.9ns    272.4ns   100.0ns        141.1ns        295.7ns    1.3μs     LaurentPolynomial
   n │ 10.5ns     48.9ns      50.0ns      49.9ns      40.2ns    216.4ns   76.7ns         77.1ns         61.4ns     63.9ns    PnPoly
   o │ 13.7ns     86.7ns      43.3ns      48.5ns      64.6ns    247.6ns   95.1ns         52.0ns         112.9ns    155.9ns   PnPoly
   
   n │ 175.5ns    131.3ns     529.8ns     529.2ns     809.5ns   9.6μs     174.0ns        569.2ns        760.2ns    651.2ns   SparsePolynomial
   o │ 178.7ns    138.1ns     428.2ns     906.0ns     815.0ns   1.8μs     174.7ns        468.7ns        856.3ns    781.6ns   SparsePolynomial

TODO:

[X] bump version number to v4.0.0 (last, as we want to continue to test against downstream pacakges)
[X] currently common.jl and abstract-polynomial.jl do the same thing and should be consolidated. Mostly done, but not all.
[x] the types Polynomial, ImmutablePolynomial, SparsePolynomial, LaurentPolynomial, PnPolynomial can be removed
[x] The Chebyshev polynomial example needs to be rewritten to use MutableDensePolynomial as a backend
[x] use this to close #510 and #512. (#511 to be addressed later
[x] check that this doesn't break downstream packages!!
[x] replace Polynomial type using new framework
[x] makes FactoredPolynomial <: AbstractPolynomial
[x] drop polynomials/standardbasis.jl (merging into standard-basis/standardbasis.jl)
[x] clean up documentation

@jverzani jverzani mentioned this pull request Aug 7, 2023
@jverzani jverzani merged commit 27d548f into JuliaMath:master Aug 15, 2023
19 of 21 checks passed
@jverzani jverzani deleted the bandaid branch August 15, 2023 17:28
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.

ImmutablePolynomial constructors seem to prevent good type inference
1 participant