Zeros and OneElement addition/subtraction #422
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #422 +/- ##
===========================================
+ Coverage 0.00% 97.71% +97.71%
===========================================
Files 8 9 +1
Lines 1152 1224 +72
===========================================
+ Hits 0 1196 +1196
+ Misses 1152 28 -1124 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
max-vassili3v
changed the title
dlfivefifty
requested changes
Dec 5, 2025
|
|
||
| # OneElements with different indices should return | ||
| # SparseArrays under addition | ||
| function FillArrays.oneelement_addsub(a::OneElementVector, b::OneElementVector, aval, bval) |
Member
There was a problem hiding this comment.
I'm not sure whether having the behaviour depend on loading an extension is a good idea...
| ret = copy(b) | ||
| try | ||
| ret[a.ind...] += getindex_value(a) | ||
| catch |
Member
There was a problem hiding this comment.
This is a non-standard idiom and introduces a type instability
dlfivefifty
requested changes
Dec 10, 2025
| catch | ||
| # Fallback to materialising dense array if setindex! | ||
| # goes wrong (e.g on a Diagonal) | ||
| ret = Array(ret) |
|
|
||
| @test B - B isa OneElement | ||
| @test B - B == OneElement(0, 2, 5) | ||
| @test B - A == [0, -1, 1, 0, 0] |
Member
There was a problem hiding this comment.
Add test with adding AbstractArray
| B = OneElement(3, 5) | ||
|
|
||
| @test A + A isa OneElement | ||
| @test A + A == OneElement(2, 2, 5) |
Member
There was a problem hiding this comment.
Suggested change
| @test A + A == OneElement(2, 2, 5) | |
| @test @inferred(A + A) == OneElement(2, 2, 5) |
| nzind = ntuple(i -> [a.ind[i], b.ind[i]], ndims(a)) | ||
| return sparse(nzind..., nzval, size(a)...) | ||
| end | ||
|
|
Member
There was a problem hiding this comment.
ADd one element + sparse array special case
| -(a::AbstractArray, b::OneElement) = sub_one_elem(a, b) | ||
| -(a::OneElement, b::AbstractArray) = sub_one_elem(a, b) | ||
| # disambiguity | ||
| function +(a::AbstractZeros, b::OneElement) |
Member
There was a problem hiding this comment.
Should we be overloading broadcast + too?
| if a.ind == b.ind | ||
| return OneElement($op(getindex_value(a), getindex_value(b)), a.ind, axes(a)) | ||
| else | ||
| return oneelement_addsub(a, b, getindex_value(a), $bop) |
Member
|
Main suggestions:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request alters some behaviour of addition/subtraction of the
ZerosandOneElementtypes. It ensures that the sparsity of any array structure that undergoes addition or subtraction with one of these types preserves its sparsity by avoiding the materialisation of dense arrays where possible:-Adding or subtracting a
Zerosand any arrayvnow preserves the type ofv(while still obeying type promotion rules)-Adding or subtracting a
OneElementto any arrayvis now O(1) and preserves the type ofvunless the underlyingsetindex!operation is invalid (e.g adding aOneElementwith a nondiagonal entry to aDiagonal), in which case a dense array is materialised-Adding or subtracting two
OneElementsnow returns aOneElementif their indices are the same. Otherwise it materialises a dense vector. IfSparseArraysis imported, it instead creates a SparseArray.Changes have been covered with unit tests