Assume mutability for canonical #31

blegat · 2024-05-21T08:18:01Z

As we discussed, since we assume the coefficients to be mutable, it's best to just call operate! and not go through the mutability logic.
I got the allocation tests starting to fail when calling operate! instead of operate!! which made no sense.
It turned out that the Julia's heuristic for specializing the type of arguments being functions changed its mind so I had to force it to specialize.

blegat · 2024-05-21T08:33:25Z

test/caching_allocations.jl

@@ -51,6 +51,8 @@ end
        @test Y * Y isa AlgebraElement
        Y * Y
        k2 = @allocated Y * Y
+        @show k1
+        @show k2
        @test k2 / k1 < 0.5


Not sure why we are checking the value of this ratio ^^

so that we know the caching had effect (i.e. reduced the number of allocations).

Should it then be < 1.0 ? what was it before?

I didn't change it, I left it at 0.5

kalmarek · 2024-05-27T15:12:44Z

test/caching_allocations.jl

@@ -51,6 +51,8 @@ end
        @test Y * Y isa AlgebraElement
        Y * Y
        k2 = @allocated Y * Y
+        @show k1
+        @show k2
        @test k2 / k1 < 0.5


so that we know the caching had effect (i.e. reduced the number of allocations).

Should it then be < 1.0 ? what was it before?

kalmarek · 2024-05-27T15:13:10Z

test/caching_allocations.jl

+        @show k1
+        @show k2


I guess we don't need those @shows ;)

I added them to see what ratio we can now use

codecov · 2024-05-27T15:25:29Z

Codecov Report

Attention: Patch coverage is 93.93939% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 93.65%. Comparing base (06977b1) to head (c364d06).
Report is 4 commits behind head on mk/non_monomial_basis.

❗ Current head c364d06 differs from pull request most recent head 7b6964d

Please upload reports for the commit 7b6964d to get more accurate results.

Files	Patch %	Lines
src/coefficients.jl	87.38%	14 Missing ⚠️
src/mtables.jl	91.25%	7 Missing ⚠️
src/diracs_augmented.jl	91.78%	6 Missing ⚠️
src/mstructures.jl	86.66%	4 Missing ⚠️
src/sparse_coeffs.jl	96.38%	3 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                    @@
##           mk/non_monomial_basis      #31      +/-   ##
=========================================================
- Coverage                  94.29%   93.65%   -0.65%     
=========================================================
  Files                         14       14              
  Lines                        631      630       -1     
=========================================================
- Hits                         595      590       -5     
- Misses                        36       40       +4

Flag	Coverage Δ
unittests	`93.65% <93.93%> (-0.65%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

blegat · 2024-05-27T15:44:54Z

For Ubuntu

k1 = 28154117
k2 = 18786592
FixedBasis caching && allocations: Test Failed at /home/runner/work/StarAlgebras.jl/StarAlgebras.jl/test/caching_allocations.jl:56
  Expression: k2 / k1 < 0.5
   Evaluated: 0.6672769030547113 < 0.5

For Mac OS

k1 = 28128821
k2 = 18786592
FixedBasis caching && allocations: Test Failed at /Users/runner/work/StarAlgebras.jl/StarAlgebras.jl/test/caching_allocations.jl:56
  Expression: k2 / k1 < 0.5
   Evaluated: 0.6678769792733226 < 0.5

For Windows

k1 = 28154181
k2 = 18786592
FixedBasis caching && allocations: Test Failed at D:\a\StarAlgebras.jl\StarAlgebras.jl\test\caching_allocations.jl:56
  Expression: k2 / k1 < 0.5
   Evaluated: 0.6672753862028521 < 0.5

Let's use 0.7 ?

kalmarek · 2024-05-27T16:25:45Z

@blegat then it's a regression. On my branch I have

┌ Info: allocated
│   k1 = 8414098
│   k2 = 983136
└   k2 / k1 = 0.11684389699288028

compare k1 = 28_154_117 with k1 = 8_414_098 and k2 = 18_786_592 with k2 = 983_136.

how much does canonicalization now allocate?

blegat · 2024-05-27T18:59:04Z

We might need to force specializations on move functions like I did in 8f3fb92

* remove CachedMTable * introduce ImplicitBasis and ExplicitBasis * rename TrivialMStructure → LazyMStructure * remove baseless StarAlgebras * AlgebraElement coefficients don't need to be vector * first try on coefficients * Fixes * rename Basis → Fixed Basis * introduce DiracBasis <: ImplicitBasis * move mul! to mstructures * fix the order {K,V} of params in AbstractCoefficients * fix printing of LazyMStructure * add keytype, valtype for AbstractCoefficients * add getindex for LazyMStructure over DiracBasis * add == for AbstractCoefficients * make DiracBasis threadsafe * give it a try with different parametrization of MStructures * Fixes * move star_of to FixedBasis * return DiracDeltas on indexing DiracBasis * dispatch star through star(basis, coeffs) * move fmac! so that StarAlgebras actually load * implement basic canonicalize * more star dispatch * simplify AlgebraElement * fix: define MA.operate!(zero, ::SparseVector) * fix show methods * fix getindex for LazyMStructure * move coefficients to their separate files * move Dirac/FixedBases to their separate files * add (Dirac|Augmented)MStructure * reimplement MTables * fix the definitions of of StarAlgebra/AlgebraElements * add a new implementation of star * reimplement augmentation * various small fixes * add fmac! for AbstractVector * remove show for DiracMStructure now that it is basis free * quickly fix one * commit tmp.jl as an example of what works * move tmp.jl to test/perm_grp_algebra.jl * turn demo perm_grp_algebra.jl into testset * disable the old test for now * move compat section for Groups etc to test/Project.toml * Let AlgebraElement be an AbstractMutable * zero!(.) -> MA.operate!(zero, .) * neg! -> operate_to!(- * add! -> operate_to!(+ * mul! -> MA.operate_to! * mul! with mstruct to MA * sub! -> MA * fmac -> MA * Check for alias * unsafe_add! -> UnsafeAddMul * Document UnsafeAddMul * Fixes for Chebyshev (#23) * Fixes for Chebyshev * Add key_type * replace keytype → key_type * rename _nzpairs → nonzero_pairs * implement canonicalization via MA.operate!!(canonical, ...) * move the mutable API to coefficients * remove Dirac and use GroupElements directly This is akin to using monomials * rename AugmentedDirac to Augmented * fix: == for SparseCoefficients * reorganize defs for star, aug and norm/dot * implement MA API for SparseCoefficients * add zero_coeffs(::Type, ::AbstractBasis) currently defaults to * SparseCoefficients (ImplicitBasis) * SparseVectir (ExplicitBasis) * fix: use nonzero_pairs instead of pairs * fix: logic for canonical(::SparseCoefficients) * add getindex/setindex! for SparseCoefficients * reshuffle definitions in types.jl (this file needs to be broken down anyway) * one/zero & isone/iszero are surprisingly hard... * various small fixes/convenience additions * fix: use getindex instead of broadcasting * [tests] implement iteration over free words * [test] rewrite testset Arithmetic in group algebra * [tests] rewrite tests with free monoid algebra * [tests] rewrite tests for chaching multiplication * [tests] update constructors and sohs in group algebra * [tests] reenable tests * fix: test LinearAlgebra.dot * fix includes * bump version to 0.3 * [tests] fix: @Allocations exist in julia ≥ 1.9 * tidy up aug, star and adjoint * allow no cache in FixedBasis * document minimal AbstractCoefficients API * de-randomize !isone test * add default cache parameter (UInt32(0)) for FixedBasis * fix: compute hash on canonicalized coeffs * add Comparable to pass < for sorting in sparse coeffs * caching for MTables does not work for isbits elements This is because isbits elements may not track isdefined / isassigned status. As a consequence Matrix{T}(undef, dims) is initialized to garbage. * fix: use proper (double) indexing when evaluating AlgebraElement * enable canonical for Vectors * add += with MTable for AbstractVectors * fix: MA.operate!(zero, ...) failed for SparseVector{Rational...} Most probably due to Rationals initialized from random memory * remove old/unused functions * add Base.keys(b::ExplicitBasis) * expand docstring for AbstractBasis * inlining nonzero_pairs saves lots of allocations * test aug for Augmented basis * reduce allocations by hinting size * unbreak tests with MutableArithmetics 1.4.3 * test allocations only on 1.10 the number of allocations on nightly increased due to the introduction of Memory{T} in base julia * rearrange tests * lock complete! of MTable behind a SpinLock * Unbreak broken tests * Exclude on Julia latest * Missing newline * remove Comparable * update README with working examples * fix (?) for multithreading access to MTable cache * make augmented stuff a self-contained file * don't export supp * fix nonzero_pairs for a vector * add tests for arithmetic on coeffs * add tests for using dense vector as coeffs * add test for abstract coeffs * add a bit of docs what AbstractCoefficients is * formatting * increase coverage by removing old methods * fix norm/dot and provide coverage * fix iteration on AugmentedBasis and add tests * move the generic MA.operate! from ACoeffs to the lib * one more broken test * Base._return_type -> MA.promote_operation * Fix broken all tests but one * Turn last broken test into a passable test * Add eachindex back * Assume mutability for canonical (#31) * Assume mutability for canonical * Fix allocation test * Debug for Julia v1.6 * Use 0.7 --------- Co-authored-by: Benoît Legat <benoit.legat@gmail.com>

blegat commented May 21, 2024

View reviewed changes

kalmarek approved these changes May 27, 2024

View reviewed changes

blegat force-pushed the bl/canonical branch 2 times, most recently from ec29d80 to c364d06 Compare May 27, 2024 15:23

blegat added 3 commits May 27, 2024 17:26

Assume mutability for canonical

789bf7a

Fix allocation test

8f3fb92

Debug for Julia v1.6

033c4c9

blegat force-pushed the bl/canonical branch from c364d06 to 033c4c9 Compare May 27, 2024 15:27

Use 0.7

7b6964d

blegat merged commit 69f107e into mk/non_monomial_basis May 27, 2024
20 checks passed

kalmarek mentioned this pull request May 27, 2024

the new canonical allocates #36

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assume mutability for canonical #31

Assume mutability for canonical #31

blegat commented May 21, 2024 •

edited

Loading

blegat May 21, 2024

kalmarek May 27, 2024 •

edited

Loading

blegat May 27, 2024

kalmarek May 27, 2024 •

edited

Loading

kalmarek May 27, 2024

blegat May 27, 2024

codecov bot commented May 27, 2024 •

edited

Loading

blegat commented May 27, 2024 •

edited

Loading

kalmarek commented May 27, 2024

blegat commented May 27, 2024

Assume mutability for canonical #31

Assume mutability for canonical #31

Conversation

blegat commented May 21, 2024 • edited Loading

blegat May 21, 2024

Choose a reason for hiding this comment

kalmarek May 27, 2024 • edited Loading

Choose a reason for hiding this comment

blegat May 27, 2024

Choose a reason for hiding this comment

kalmarek May 27, 2024 • edited Loading

Choose a reason for hiding this comment

kalmarek May 27, 2024

Choose a reason for hiding this comment

blegat May 27, 2024

Choose a reason for hiding this comment

codecov bot commented May 27, 2024 • edited Loading

Codecov Report

blegat commented May 27, 2024 • edited Loading

kalmarek commented May 27, 2024

blegat commented May 27, 2024

blegat commented May 21, 2024 •

edited

Loading

kalmarek May 27, 2024 •

edited

Loading

kalmarek May 27, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading

blegat commented May 27, 2024 •

edited

Loading