Skip to content

fix for failing test in allocations - #38

Open
kalmarek wants to merge 9 commits into
mainfrom
mk/fix_allocs
Open

fix for failing test in allocations#38
kalmarek wants to merge 9 commits into
mainfrom
mk/fix_allocs

Conversation

@kalmarek

@kalmarek kalmarek commented May 30, 2024

Copy link
Copy Markdown
Collaborator

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

codecov Bot commented May 30, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.70%. Comparing base (5bd4ae5) to head (57a74ab).
⚠️ Report is 88 commits behind head on main.

Files with missing lines Patch % Lines
src/arithmetic.jl 33.33% 6 Missing ⚠️
Additional details and impacted files
@@            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 Coverage Δ
unittests 92.70% <72.72%> (-0.82%) ⬇️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@blegat

blegat commented May 31, 2024

Copy link
Copy Markdown
Member

@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

blegat commented May 31, 2024

Copy link
Copy Markdown
Member

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

🎉 I turned them to not broken

@kalmarek

kalmarek commented Jun 1, 2024

Copy link
Copy Markdown
Collaborator Author

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

blegat commented Jun 3, 2024

Copy link
Copy Markdown
Member

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

kalmarek commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator Author

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).

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.

the new canonical allocates

2 participants