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

Implement efficient sparse array comparisons #227

Merged
merged 14 commits into from
Aug 23, 2022

Conversation

eschnett
Copy link
Contributor

Closes #226.

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2022

Codecov Report

Merging #227 (fd508a1) into main (f51b64c) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
+ Coverage   92.11%   92.15%   +0.03%     
==========================================
  Files          11       11              
  Lines        7203     7223      +20     
==========================================
+ Hits         6635     6656      +21     
+ Misses        568      567       -1     
Impacted Files Coverage Δ
src/sparsematrix.jl 95.35% <100.00%> (+0.02%) ⬆️
src/higherorderfns.jl 96.69% <0.00%> (+0.02%) ⬆️
src/linalg.jl 84.58% <0.00%> (+0.08%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

eschnett and others added 2 commits August 19, 2022 23:01
@SobhanMP
Copy link
Member

Is there a need to check zero(eltype(A)) == zero(eltype(B)) ? or is this assumed to be true regardless?

@rayegun
Copy link
Member

rayegun commented Aug 20, 2022

SparseMatrixCSC assumes that the fill value is zero(T) broadly speaking, IIRC.

@eschnett
Copy link
Contributor Author

@SobhanMP Yes, that needs to be checked, since e.g. 0 == [0] isn't true.

@SobhanMP
Copy link
Member

@Wimmerer yes but are all zeros equal?

@eschnett eschnett marked this pull request as draft August 22, 2022 02:47
@eschnett eschnett marked this pull request as ready for review August 22, 2022 03:39
@eschnett eschnett mentioned this pull request Aug 22, 2022
B = Counting.(B)
As = Any[A, A', transpose(A)]
Bs = Any[B, B', transpose(B)]
for A′ in As, B′ in Bs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotta say the github font is bad 😅 .

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saved by the colour coding?

@SobhanMP
Copy link
Member

SobhanMP commented Aug 22, 2022

so i compared the number of operations with flattened arrays and it seems that this patch does less comparison than sparse sparse comparison. let me check a few things tonight. otherwise looks good.

@ViralBShah
Copy link
Member

If all good, then please merge.

@SobhanMP
Copy link
Member

SobhanMP commented Aug 22, 2022

in the end i think it's best to not compare zero, for the sake of compability with ==. i also made the test stricter. i.e. nnz(A) + nnz(B) + 1 operations at max. also some style upgrade.

@ViralBShah do you know how it would be possible to make a benchmark for this package. to track performance degradations?

@eschnett
Copy link
Contributor Author

I think we should compare the zeros:

julia> fill(0,1,1) == fill([0;;],1,1)
false

julia> sparse(fill(0,1,1)) == sparse(fill([0;;],1,1))
true

That's a bug in the current sparse array comparisons.

@eschnett
Copy link
Contributor Author

Opened #234 for the issue above.

@SobhanMP
Copy link
Member

i'll go ahead an merge this, if any one has any opinion, i guess we can move to #234

@SobhanMP SobhanMP merged commit fca26c4 into JuliaSparse:main Aug 23, 2022
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.

Comparing sparse matrices to adjoints is very slow
5 participants