-
-
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
Make *Triangular
handle units
#43972
Conversation
7034628
to
fe2a6df
Compare
@nanosoldier |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
end | ||
c[i] = A[i,i] \ bi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a more efficient way to move the correct physical dimensions to bi
?
When the element types of A
and b
are unitful integers, the result could also be unitful integers, without any floating point operations performed (no division by 1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of any. convert
and alike fail because you can't convert, for instance, square meters to meters or a unitless number. Also, I'm not sure if there is a generic way of identifying whether a unitful quantity is integer-based. For unitless numbers you can see that I made an effort to preserve the integer return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worked around the issue. First, the target c
is initialized with an integer unitful eltype, and second I replaced the division by a multiplication. I'll add a test for that also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, no. For unitful quantities, we need to "perform" the division to get the right unit: assume both A
and b
have integer units of kilometers, then the result is "unit-free". In terms of types, this means (Int*km) \ (Int*km) = Float64
, otherwise the kilometers don't cancel out.
fa2fd1a
to
6367cc4
Compare
c2668c7
to
61d5fd3
Compare
@nanosoldier |
This reverts commit 4038d8d.
Feedback. 😜 This is a pretty big refactoring, and it increases CI runtime in the triangular test file (due to compilation of more methods) and in the bidiag test file due to quite some more tests involving units. I'm wondering if this is all worth it or if we have other options to handle unitful linear algebra (and whether this is all of sufficient interest). |
@nanosoldier |
Your package evaluation job has completed - possible new issues were detected. |
@nanosoldier |
Your package evaluation job has completed - possible new issues were detected. |
pkgeval says we're good. So let's go? |
@nanosoldier |
@nanosoldier |
Your benchmark job has completed - successfully executed benchmarks. A full report can be found here. |
@nanosoldier |
Your benchmark job has completed - successfully executed benchmarks. A full report can be found here. |
I must have missed some update for nanosoldier. We used to have green checkmarks and red crosses in the benchmarks output. How do I read the output now? Does the absence of these signs mean "everything within expectation"? @maleadt @KristofferC |
That's because you didn't do a comparison benchmark. Only the PkgEval Nanosoldier instance supports leaving off the |
I wasn't reading carefully, but just looked at the examples at https://github.com/JuliaCI/Nanosoldier.jl, and copy-pasted. Now I see way below that there are more details to be specified. Let's try again. @nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
The only reported regression must be fake. The code path ( |
This is an attempt to make triangular linear algebra handle unitful objects well, like (most of)
UpperHessenberg
andBidiagonal
. The first commit rewrites the generic functions in terms of 3-argmul!
,ldiv!
and the internal_rdiv!
and doesn't change any tests to convince ourselves that this doesn't change behavior. 2-arg versions for BLAS-able argument types remain fully functional.Some of the loops had to be reorder to keep the variables consistent with units.Closes JuliaLang/LinearAlgebra.jl#875.
EDIT (copied from below):
The features of the PR are:
mul!(::Matrix{T}, ::Matrix{T}, ::AbstractTriangular{T, <:Matrix{T}}) where T<:BlasFloat
does not use BLAS LinearAlgebra.jl#875)mul!
methods down to 55 (we have 138 in v1.9)_ustrip
" mechanism that packages can hook into to have generic substitution codes run without overhead by division by someoneunit