JuliaLinearAlgebra / Octavian.jl

Multi-threaded BLAS-like library that provides pure Julia matrix multiplication
https://julialinearalgebra.github.io/Octavian.jl/stable/
Other
226 stars 18 forks source link

update Requires.jl code to package extensions #175

Closed ranocha closed 1 year ago

ranocha commented 1 year ago

Tests run fine locally on Julia v1.9.0-rc1

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 99.02% and project coverage change: -0.33 :warning:

Comparison is base (cacaf1e) 86.45% compared to head (06962d1) 86.12%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #175 +/- ## ========================================== - Coverage 86.45% 86.12% -0.33% ========================================== Files 13 13 Lines 1004 1002 -2 ========================================== - Hits 868 863 -5 - Misses 136 139 +3 ``` | [Impacted Files](https://codecov.io/gh/JuliaLinearAlgebra/Octavian.jl/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra) | Coverage Δ | | |---|---|---| | [src/Octavian.jl](https://codecov.io/gh/JuliaLinearAlgebra/Octavian.jl/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra#diff-c3JjL09jdGF2aWFuLmps) | `0.00% <ø> (-50.00%)` | :arrow_down: | | [src/init.jl](https://codecov.io/gh/JuliaLinearAlgebra/Octavian.jl/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra#diff-c3JjL2luaXQuamw=) | `84.61% <ø> (-0.57%)` | :arrow_down: | | [ext/ForwardDiffExt.jl](https://codecov.io/gh/JuliaLinearAlgebra/Octavian.jl/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra#diff-ZXh0L0ZvcndhcmREaWZmRXh0Lmps) | `99.02% <99.02%> (ø)` | | ... and [2 files with indirect coverage changes](https://codecov.io/gh/JuliaLinearAlgebra/Octavian.jl/pull/175/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaLinearAlgebra)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

ranocha commented 1 year ago

Perhaps drop Requires.jl altogether, and load the file unconditionally on Julia 1.8 and older?

I'm not sure. I think the current approach is still fine since it does not load ForwardDiff.jl code when ForwardDiff.jl is not loaded. Thus, people not using ForwardDiff.jl will have increased latency.

chriselrod commented 1 year ago

I think the current approach is still fine since it does not load ForwardDiff.jl code when ForwardDiff.jl is not loaded

ForwardDiff.jl will always be loaded on Julia 1.8 and older, because LoopVectorization.jl, which Octavian.jl depends on, always loads ForwardDiff on Julia 1.8 and older.

So, to cut latency, cut Requires.jl.

ranocha commented 1 year ago

Sounds reasonable. Is d5ac6bf what you had in mind?

chriselrod commented 1 year ago

Sounds reasonable. Is d5ac6bf what you had in mind?

Yes.

ranocha commented 1 year ago

CI on Julia nightly errors with

Aqua.jl: Test Failed at /home/runner/work/Octavian.jl/Octavian.jl/test/aqua.jl:4
  Expression: isempty(Test.detect_ambiguities(Octavian))
   Evaluated: isempty(Tuple{Method, Method}[
  (_matmul!(_C::AbstractVecOrMat{DualT}, _A::AbstractMatrix{DualT}, B::AbstractVecOrMat, α, β, nthread::Nothing, MKN) where {TAG, T, DualT<:(Dual{TAG, T})} @ ForwardDiffExt ~/work/Octavian.jl/Octavian.jl/ext/ForwardDiffExt.jl:45, 
   _matmul!(y::AbstractVector{T}, A::AbstractMatrix, x::AbstractVector, α, β, ::Any, ::Any) where T @ Octavian ~/work/Octavian.jl/Octavian.jl/src/matmul.jl:960), 
  (_matmul_serial!(_C::AbstractVecOrMat{DualT}, _A::AbstractMatrix{DualT}, B::AbstractVecOrMat, α, β, MKN) where {TAG, T, DualT<:(Dual{TAG, T})} @ ForwardDiffExt ~/work/Octavian.jl/Octavian.jl/ext/ForwardDiffExt.jl:154, 
   _matmul_serial!(y::AbstractVector{T}, A::AbstractMatrix, x::AbstractVector, α, β, ::Any) where T @ Octavian ~/work/Octavian.jl/Octavian.jl/src/matmul.jl:978)])

It looks like we would need to define

_matmul!(_C::AbstractVector{DualT}, _A::AbstractMatrix{DualT}, B::AbstractVector, α, β, nthread::Nothing, MKN) where {TAG, T, DualT<:(Dual{TAG, T})}
_matmul!(_C::AbstractMatrix{DualT}, _A::AbstractMatrix{DualT}, B::AbstractMatrix, α, β, nthread::Nothing, MKN) where {TAG, T, DualT<:(Dual{TAG, T})}

and similarly for _matmul_serial! to resolve the ambiguities. Shall I do this or shall we postpone this since it hapens only on Julia nightly?

chriselrod commented 1 year ago

Mind replacing the AbstractVecOrMat with an @eval loop over AbstractVector and AbstractMatrix?

I know that is a bad thing to do. The cost of adding methods is worse than O(N), so the marginal cost of extra methods increases. This causes problems with packages adding methods to LinearAlgebra.mul! being extremely costly.

_matmul! at least should be cheap.

You're welcome to address this another way (e.g., address the AbstractVector _matmul specialization), but this seems fastest/easiest.

ranocha commented 1 year ago

Done in cb46881

ranocha commented 1 year ago

The multi-threaded Windows CI job ran out of memory https://github.com/JuliaLinearAlgebra/Octavian.jl/actions/runs/4799970278/jobs/8540299392?pr=175#step:6:299

chriselrod commented 1 year ago

The multi-threaded Windows CI job ran out of memory https://github.com/JuliaLinearAlgebra/Octavian.jl/actions/runs/4799970278/jobs/8540299392?pr=175#step:6:299

I restarted it. Octavian's settings don't let me ignore test failures.

ranocha commented 1 year ago

Thanks!

chriselrod commented 1 year ago

Thanks for the PR!