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

WIP: Explicit basis #513

Closed
wants to merge 32 commits into from
Closed

Conversation

jverzani
Copy link
Member

@jverzani jverzani commented Jul 20, 2023

Currently the polynomial type implies both a basis and a backend storage type. E.g., ImmutablePolynomial uses the standard basis and a tuple for storage. This PR breaks this up so the basis is explicit -- the equivalent being ImmutableDensePolynomial{StandardBasis}. If all goes to plan, this will be aliased to ImmutablePolynomial and the PR should be non breaking to the end user.

The main types are

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

Some simple benchmarking shows some 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  types         ⋯
     │ String     String      String      String      String    String    String         String         String     String    String        ⋯
─────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
*  1 │ 10.5ns     61.2ns      54.8ns      55.2ns      70.1ns    221.4ns   87.0ns         81.8ns         131.2ns    149.8ns   MutableDenseP ⋯
   2 │ 13.7ns     46.0ns      44.7ns      91.3ns      65.9ns    242.9ns   73.4ns         69.3ns         353.0ns    397.4ns   Polynomial
   3 │ 11.3ns     77.5ns      115.0ns     113.5ns     79.5ns    272.9ns   100.0ns        139.9ns        303.2ns    1.3μs     LaurentPolyno
*  4 │ 32.8ns     27.4ns      33.7ns      34.6ns      25.0ns    97.9ns    26.9ns         35.3ns         36.8ns     35.8ns    ImmutableDens
   5 │ 33.3ns     31.1ns      33.6ns      1.3μs       29.5ns    101.3ns   26.1ns         29.7ns         1.6μs      1.7μs     ImmutablePoly ⋯
*  6 │ 192.1ns    135.0ns     429.1ns     423.0ns     782.4ns   9.8μs     173.8ns        476.6ns        771.6ns    796.7ns   MutableSparse
   7 │ 192.9ns    139.7ns     420.1ns     911.5ns     787.2ns   10.3μs    173.4ns        453.9ns        865.8ns    792.1ns   SparsePolynom

TODO:

[] currently common.jl and abstract-polynomial.jl do the same thing and should be consolidated. (For later, if at all)
[x] the types ImmutablePolynomial, SparsePolynomial, LaurentPolynomial can be removed (Polynomial too)
[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!!

The plan is for this to be followed by another PR which should
[] replaces Polynomial type using new framework
[] makes FactoredPolynomial <: AbstractPolynomial
[] drop polynomials/standardbasis.jl (merging into standard-basis/standardbasis.jl)

@jverzani jverzani changed the title Explicit basis WIP: Explicit basis Jul 20, 2023
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage: 85.48% and project coverage change: -1.89% ⚠️

Comparison is base (f4d90d9) 79.78% compared to head (7b3fe93) 77.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
- Coverage   79.78%   77.90%   -1.89%     
==========================================
  Files          26       35       +9     
  Lines        3216     3838     +622     
==========================================
+ Hits         2566     2990     +424     
- Misses        650      848     +198     
Files Changed Coverage Δ
src/Polynomials.jl 100.00% <ø> (ø)
src/rational-functions/common.jl 71.63% <ø> (ø)
src/standard-basis/polynomial.jl 0.00% <0.00%> (ø)
src/abstract.jl 49.12% <20.00%> (-4.73%) ⬇️
src/promotions.jl 33.33% <33.33%> (ø)
src/show.jl 76.74% <36.36%> (-4.09%) ⬇️
...ynomial-basetypes/mutable-dense-view-polynomial.jl 40.00% <40.00%> (ø)
src/polynomials/multroot.jl 57.95% <44.44%> (+0.32%) ⬆️
src/polynomials/standard-basis.jl 74.41% <44.44%> (-5.95%) ⬇️
.../polynomial-basetypes/mutable-sparse-polynomial.jl 81.48% <81.48%> (ø)
... and 14 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jverzani
Copy link
Member Author

jverzani commented Aug 7, 2023

Closing, cf #517

@jverzani jverzani closed this Aug 7, 2023
@jverzani jverzani deleted the explicit_basis branch August 7, 2023 20:44
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