-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Improve consistency of similar methods operating on sparse matrices and flesh them out #17507
Conversation
2ee5664
to
bf97e7f
Compare
The AV i686 failure appears unrelated, but the Travis i686 failure appears real. I can't reproduce the latter locally though, and inspecting the test output
suggests that the result is correct, but perhaps not within some tolerance? Thanks! |
Hi. I'm wondering if there's still a plan to fix the CI issues with this PR and get it merged? I just ran into #17456 on Julia 0.5.1 (commit a6c55c5*). Also checked that it still exists on master (commit d6088fc*). I feel a bit silly asking about this when I don't know enough to debug why the CI checks are failing but I thought I'd check in to see if it's just been forgotten about. Is it worth merging this work into an up to date version of |
Rewritten. What say you now, CI? |
Possible alternative: have similar return a reshaped sparse
|
I guess, sure, but doesn't this PR also implicitly fix #23905 by removing the definition for |
Yes, as a side effect this pull request partially addresses #23906, and the sequel for |
All test failures appear unrelated, except for the CircleCI i686 failure, which is bizarre: Some tests did not pass: 1817 passed, 1 failed, 0 errored, 0 broken.sparse/sparse: Test Failed
Expression: transpose(view(speye(10), 1:5, 1:5)) ≈ eye(5, 5)
Evaluated:
[1, 1] = 1.0
[2, 2] = 1.0
[3, 3] = 1.0
[4, 4] = 1.0
[5, 5] = 1.0 ≈ [1.0 0.0 0.0 0.0 0.0; 0.0 1.0 0.0 0.0 0.0; 0.0 0.0 1.0 0.0 0.0; 0.0 0.0 0.0 1.0 0.0; 0.0 0.0 0.0 0.0 1.0] On the one hand, this failure is quite similar to the failure that blocked this pull request better than a year ago on Travis i686: The failure is also on i686, the report suggests that the test should pass, and I cannot reproduce the failure locally. On the other hand, and what makes the failure seem particularly strange, is that Travis i686 now passes. Any insight would be much appreciated! |
(Seems the failure was either unrelated or related but nondeterministic (though the test is deterministic), as CircleCI i686 passed this round.) |
Another form of the same failure appeared this round on Travis x86_64 Linux: Error in testset sparse/sparse:
Test Failed
Expression: f(arr, 1) ≈ f(farr, 1)
Evaluated:
[1, 2] = -0.47132
[1, 3] = -0.463003
[1, 4] = 0.0
[1, 5] = -0.848927
[1, 6] = -0.509547
[1, 7] = -0.478014 ≈ [0.0 -0.47132 -0.463003 0.0 -0.848927 -0.509547 -0.478014] So this issue is not confined to 32-bit platforms. I will see whether I can reproduce this locally via repeated runs. Update: On the sixth consecutive local (macOS) run of |
Reduced to an issue with |
Confirming that #24204 seems to fix the stress test I have locally exercising the nondeterministic CI failure. Cycling CI. Best! |
Makes similar methods for SparseMatrixCSC more consistent both among themselves and with similar methods for other types. Also fleshes those methods out in full.
Absent objections or requests for time, I plan to merge these changes this evening (assuming CI clears). Best! |
Thanks all! :) |
This pull request's first commit: (1) makes
similar
methods operating onSparseMatrixCSC
s more consistent both among themselves and with othersimilar
methods in Base; and (2) fleshes those methods out. The second commit thoroughly tests thosesimilar
methods. The first commit fixes #17456, and the second covers (tests) #17456's underlying cause. Best!