search

JuliaAlgebra / StarAlgebras.jl

A package for computation in *-algebras with basis
MIT License
7 stars 3 forks source link

fix for failing test in allocations #38

Open kalmarek opened 5 months ago

kalmarek commented 5 months ago

as suspected calling canonical on Tuple-based SparseCoefficients is not a good idea. And these are actually immutable. For the time being adding this one overload brings back the allocations AND performance of *.

Fixes #36

@blegat is the mutability trait even needed now?

EDIT: test will fail due to broken test now fixed.

codecov[bot] commented 5 months ago

Codecov Report

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

Project coverage is 92.70%. Comparing base (5bd4ae5) to head (57a74ab).

Files Patch % Lines
src/arithmetic.jl 33.33% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #38 +/- ## ========================================== - Coverage 93.51% 92.70% -0.82% ========================================== Files 14 14 Lines 632 644 +12 ========================================== + Hits 591 597 +6 - Misses 41 47 +6 ``` | [Flag](https://app.codecov.io/gh/JuliaAlgebra/StarAlgebras.jl/pull/38/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAlgebra) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/JuliaAlgebra/StarAlgebras.jl/pull/38/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAlgebra) | `92.70% <72.72%> (-0.82%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaAlgebra#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

blegat commented 5 months ago

@blegat is the mutability trait even needed now?

Since we are calling operate! explicitly, it's not necessary to implement mutability indeed. However, the user could call operate!! in which case it's useful but then mutability should return IsNotMutable if the storage is a tuple. About the subtyping <:AbstractMutable, it's very useful so that we get fast matrix-vector multiplications etc... from MutableArithmetics/src/dispatch.jl

blegat commented 5 months ago

EDIT: test will fail due to broken test now fixed.

:tada: I turned them to not broken

kalmarek commented 5 months ago

very well, but I'm not sure that this is the way forward -- adding this canonical method is rather a hack. Much cleaner imo would be to return to calling operate!! where appropriate, which utilizes the mutability trait.

blegat commented 5 months ago

Why would we need operate! only for canonical, I don't quite get where this is called and why we need to canonicalize if we didn't do any operate! before

kalmarek commented 5 months ago

Then maybe we think of canonical in a very different way. For me objects manipulated (algebra elements) do not need to be in their canonical form which makes all the operations cheaper (e.g. lazy). Only when canonical form is actually needed (e.g. in ==) the objects are brought to it. On the other hand canonicalisation of an object in canonical form should be as cheap as possible (possibly as fast as comparison of a Bool).