Skip to content
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

Add missing matrix multiplication methods involving OneElement #347

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Feb 1, 2024

This adds several missing methods, so

julia> @btime OneElement(3, (3,3), (50,50)) * $(ones(50));
  8.797 μs (1 allocation: 496 bytes) # master
  70.977 ns (1 allocation: 496 bytes) # PR

julia> @btime OneElement(3, (3,3), (50,50)) * $(ones(50,50));
  138.330 μs (7 allocations: 50.42 KiB) # master
  1.254 μs (2 allocations: 19.61 KiB) # PR

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9f5c6aa) 99.89% compared to head (77d751d) 99.90%.
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   99.89%   99.90%   +0.01%     
==========================================
  Files           8        8              
  Lines         934     1037     +103     
==========================================
+ Hits          933     1036     +103     
  Misses          1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oxinabox
Copy link
Member

oxinabox commented Feb 2, 2024

This PR should hit 100% code coverage.
It wouldn't be too hard to add the 6 lines that are missed as identified by code-cov

@jishnub
Copy link
Member Author

jishnub commented Feb 2, 2024

The codecov upload errored, e.g. in https://github.com/JuliaArrays/FillArrays.jl/actions/runs/7740101659/job/21104345601?pr=347. I think running the tests again might lead to more accurate results.

Comment on lines +158 to 161
if iszero(getindex_value(x))
mul!(y, A, Zeros{eltype(x)}(axes(x)), alpha, beta)
return y
end
Copy link
Member

Choose a reason for hiding this comment

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

I am dubious as to if this check is worth the potential type instability.
I feel like such a OneElement where that element is also zero is almost never constructed.

Copy link
Member Author

@jishnub jishnub Feb 2, 2024

Choose a reason for hiding this comment

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

It's not type-unstable, though. It just calls a different method that returns the same vector, and should be type-stable as well. This is a guard against the current implementation allowing the indices to not lie within the axes (which probably should be disallowed).

E.g.:

julia> @report_opt mul!(zeros(2), ones(2,2), OneElement(1,2))
No errors detected

Comment on lines +184 to +187
if iszero(getindex_value(B))
mul!(C, A, Zeros{eltype(B)}(axes(B)), alpha, beta)
return C
end
Copy link
Member

Choose a reason for hiding this comment

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

per above I am dubious

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

I am assume because the tests pass and this has 100% coverage that it is correct

@jishnub jishnub merged commit 99278fa into JuliaArrays:master Feb 2, 2024
18 checks passed
@jishnub jishnub deleted the linalgoneel branch February 2, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants