Skip to content

Conversation

mcabbott
Copy link
Contributor

This is a start on #46.

test/runtests.jl Outdated

ambig = sort(detect_ambiguities(Unitful), by = a -> [string(a[1].name), string(a[2].module)])
if length(ambig) > 0
println(stdout, "detect_ambiguities(Unitful) found $(length(ambig)) issues:")
Copy link
Contributor Author

@mcabbott mcabbott Apr 13, 2021

Choose a reason for hiding this comment

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

I was concerned about creating ambiguities, so made it print them before starting.

In the end, overloading mul! not * seems not to create problems. But there are (for me) 30 other ambiguities this detects (and prints out).

Now moved to #439 instead.

function LinearAlgebra.mul!(C::StridedVecOrMat{<:AbstractQuantity{T}},
A::StridedMatrix{<:AbstractQuantity{T}},
B::StridedVecOrMat{<:AbstractQuantity{T}},
alpha::Bool, beta::Bool) where {T<:Base.HWNumber}
Copy link
Contributor Author

@mcabbott mcabbott Apr 13, 2021

Choose a reason for hiding this comment

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

The case α, β::Bool is what A * B produces. Allowing other pure numbers ought to be fine. Allowing these to have units I haven't looked into.

I have restricted to A, B, C having the same eltype, as I think this is what BLAS handles. Julia sometimes copies a matrix to promote e.g. Float64 * Float32, but I don't recall at what stage.

5-arg mul! doesn't exist on 1.0, but that's OK, this will just never be called, so you'll get the slow but correct fallback.

@codecov-io
Copy link

codecov-io commented Apr 13, 2021

Codecov Report

Merging #437 (9515ad2) into master (cc3adef) will increase coverage by 0.12%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
+ Coverage   83.84%   83.97%   +0.12%     
==========================================
  Files          16       17       +1     
  Lines        1337     1354      +17     
==========================================
+ Hits         1121     1137      +16     
- Misses        216      217       +1     
Impacted Files Coverage Δ
src/Unitful.jl 100.00% <ø> (ø)
src/linearalgebra.jl 95.45% <95.45%> (ø)
src/utils.jl 95.34% <100.00%> (ø)
src/user.jl 94.07% <0.00%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc3adef...9515ad2. Read the comment docs.

_linearalgebra_count()
u = inv(unit(eltype(A)))
Tu = typeof(one(eltype(C0)) * u)
return reinterpret(Tu, C0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that these methods will return a ReinterpretArray{Quantity{... not a Matrix. There's no obvious analogue the mul! overloaded for A * B.

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@cc3adef). Click here to learn what that means.
The diff coverage is 51.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #437   +/-   ##
=========================================
  Coverage          ?   82.05%           
=========================================
  Files             ?       17           
  Lines             ?     1393           
  Branches          ?        0           
=========================================
  Hits              ?     1143           
  Misses            ?      250           
  Partials          ?        0           
Impacted Files Coverage Δ
src/Unitful.jl 100.00% <ø> (ø)
src/linearalgebra.jl 47.91% <47.91%> (ø)
src/utils.jl 87.50% <64.28%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc3adef...c47347a. Read the comment docs.

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.

3 participants