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.

Sign up for GitHub

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

Extend parameter trait to wmetrics #150

Merged
merged 7 commits into from
Apr 10, 2020
Merged

Extend parameter trait to wmetrics #150

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4207e42
merge wmetrics.jl into metrics.jl
dkarrasch Oct 3, 2019
acfbe99
rearrange docstrings, no functional modifications
dkarrasch Oct 3, 2019
0154de0
reduce unnecessary code after moving, add some scalar metrics, tests
dkarrasch Oct 4, 2019
69a5c16
remove wmetrics.jl
dkarrasch Oct 4, 2019
407f964
replace RealAbstractArray
dkarrasch Oct 4, 2019
dbc7aeb
edits as per review
dkarrasch Oct 14, 2019
4d76aa4
replace 1:length by eachindex
dkarrasch Oct 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/metrics.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ Base.@propagate_inbounds function _evaluate(d::UnionMetrics, a::AbstractArray, b
@inbounds begin
s = eval_start(d, a, b)
if IndexStyle(a, b) === IndexLinear() || size(a) == size(b)
@simd for I in 1:length(a)
@simd for I in eachindex(a, b)
Copy link
Member

Choose a reason for hiding this comment

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

Note JuliaLang/julia#28226 (comment).

Is that no longer the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested #150 (comment), and obtained the results above. Wasn't this the test that would determine the slowdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I meant #107 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

I get that colwise is consistently and measurably faster than naivecolwise in that test.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it is fixed then...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll quickly double-check on Julia v1.0...

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I have tested my but off, and I can't seem to be able to reproduce the slowdown anymore, even on Julia v1.0. While the issue JuliaLang/julia#28226 persists even on nightlies, the issue #100 does not re-occur with this PR, neither on Julia v1.0.5 nor v1.3 or nightlies. AFAICT, we wouldn't introduce a slowdown with the change in the line commented on above.

ai = a[I]
bi = b[I]
s = eval_reduce(d, s, eval_op(d, ai, bi))
Expand Down Expand Up @@ -251,7 +251,7 @@ Base.@propagate_inbounds function _evaluate(d::UnionMetrics, a::AbstractArray, b
@inbounds begin
s = eval_start(d, a, b)
if IndexStyle(a, b, p) === IndexLinear() || size(a) == size(b)
@simd for I in 1:length(a)
@simd for I in eachindex(a, b)
ai = a[I]
bi = b[I]
pi = p[I]
Expand Down