-
Notifications
You must be signed in to change notification settings - Fork 53
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
Change default QR tolerance to match SPQR #557
Conversation
SPQR uses just the double precision epsilon even for Float32. https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/131471310ef0600b231b8fa7c10a55c3f70afbd9/SPQR/Source/spqr_tol.cpp#L29C6-L30C57
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #557 +/- ##
==========================================
+ Coverage 84.09% 84.12% +0.03%
==========================================
Files 12 12
Lines 9102 9098 -4
==========================================
Hits 7654 7654
+ Misses 1448 1444 -4 ☔ View full report in Codecov by Sentry. |
@DrTimothyAldenDavis Should we follow the SuiteSparse lead here - just wanted your eyes once more - using |
SPQR doesn't support float. It only supports double and double-complex. There are only 4 explicit instantiations of the spqr_tol templatized function: (double or double-complex) and (int32 or int64). I'm not sure what the tol should be for a float or float-complex case. I'm guessing it should be based on the FLT_EPSILON, not DBL_EPSILON. |
Ahh silly me, I didn't realize floats weren't supported. Well I do have a use case where the default tolerance for a Float32 matrix (as computed in Julia) is ridiculously large. My intuition is that the default tolerance should be Edit: Another reason this line should be changed (although not necessarily in the way I suggest) is that it fails when qr(sparse([1 1; 1 1]))
ERROR: MethodError: no method matching eps(::Type{Int64})
...
Stacktrace:
[1] _default_tol(A::SparseMatrixCSC{Int64, Int64})
...
qr(sparse([1 1; 1 1]); tol=0)
SparseArrays.SPQR.QRSparse{Float64, Int64}
... |
I am inclined to go with @DrTimothyAldenDavis as the default suggestion (but you can always use a different tolerance). It is unclear to me what |
That's fine if you want to leave it as @ViralBShah since julia simply converts an integer matrix into a |
Ah - if we are solving in Float64 anyways, we might as well use eps(Float64). I suppose your point about the eps on integer also makes sense. Let's do both. In that case we should be able to merge this right? |
Yes if we simply want to use |
SPQR uses just the double precision epsilon even for Float32. https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/131471310ef0600b231b8fa7c10a55c3f70afbd9/SPQR/Source/spqr_tol.cpp#L29C6-L30C57
SPQR uses just the double precision epsilon even for Float32. https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/131471310ef0600b231b8fa7c10a55c3f70afbd9/SPQR/Source/spqr_tol.cpp#L29C6-L30C57
SPQR uses just the double precision epsilon even for Float32.
https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/131471310ef0600b231b8fa7c10a55c3f70afbd9/SPQR/Source/spqr_tol.cpp#L29C6-L30C57