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

Fix copyto!(dest, Rdest::CartesianIndices, src, Rsrc::CartesianIndices) with different step. #45289

Merged
merged 1 commit into from
May 16, 2022

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented May 12, 2022

Local bench shows no performance regression, if the indicess of Rdest and Rsrc are all NTuple{N,<:AbstractUnitRange}.
Close #45282.

@N5N3 N5N3 added backport 1.6 Change should be backported to release-1.6 backport 1.8 Change should be backported to release-1.8 labels May 12, 2022
@KristofferC KristofferC added kind:bugfix This change fixes an existing bug domain:arrays [a, r, r, a, y, s] status:merge me PR is reviewed. Merge when all tests are passing labels May 12, 2022
test/copy.jl Outdated Show resolved Hide resolved
There's no performance change, if the `indices`s of `Rdest` and `Rsrc` are all `NTuple{N,<:AbstractUnitRange}`.
@N5N3
Copy link
Member Author

N5N3 commented May 13, 2022

Unrelated power test failure looks like a real regression (after #42031?) CC @oscardssmith.
On master

julia> x, y = 48.718f0, -2.f0;

julia> Base.Math.pow_body(x,y)
0.0004213288f0

julia> x^y
0.00042132882f0

julia> x^-2
0.00042132882f0

Edit: My local test shows that inv(x*x) is a little bit more accurate than inv(x) * inv(x).

using Random
Random.seed!(0);
a = rand(Float32, 100000000) * 100;
f(x) = widen(inv(x)*inv(x));
g(x) = widen(inv(x*x));
c(x) = widen(x)^-2.0;
fa = f.(a);
ga = g.(a);
ca = c.(a);
maximum(x -> isnan(x) ? zero(x) : abs(x), (fa .- ca)./ca) #1.786684364953734e-7
sum(x -> isnan(x) ? zero(x) : abs(x), (fa .- ca)./ca)          #4.616936626912864
maximum(x -> isnan(x) ? zero(x) : abs(x), (ga .- ca)./ca)#8.930594822349074e-8
sum(x -> isnan(x) ? zero(x) : abs(x), (ga .- ca)./ca)         #2.965068731509999

@KristofferC KristofferC merged commit a91be39 into JuliaLang:master May 16, 2022
@oscardssmith
Copy link
Member

@N5N3 that's annoying, but I don't think there's anything good to do here. inv(x) * inv(x) is a little less accurate over most of the range, but inv(x*x) can be almost infinitely wrong if x*x overflows. For example, x=nextfloat(sqrt(floatmax(Float32))) gives 0f0 for inv(x*x), but 2.938736f-39 for inv(x)*inv(x).

Given this, I see 3 possible paths:

  1. make the test only 1.4 ULP when x==-2
  2. make x^-2 use full compensation (this would make it roughly 4x slower)

@N5N3 N5N3 removed the status:merge me PR is reviewed. Merge when all tests are passing label May 17, 2022
KristofferC pushed a commit that referenced this pull request May 18, 2022
There's no performance change, if the `indices`s of `Rdest` and `Rsrc` are all `NTuple{N,<:AbstractUnitRange}`.

(cherry picked from commit a91be39)
KristofferC pushed a commit that referenced this pull request May 18, 2022
There's no performance change, if the `indices`s of `Rdest` and `Rsrc` are all `NTuple{N,<:AbstractUnitRange}`.

(cherry picked from commit a91be39)
KristofferC pushed a commit that referenced this pull request May 23, 2022
There's no performance change, if the `indices`s of `Rdest` and `Rsrc` are all `NTuple{N,<:AbstractUnitRange}`.

(cherry picked from commit a91be39)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label May 28, 2022
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Jul 6, 2022
KristofferC pushed a commit that referenced this pull request Dec 21, 2022
There's no performance change, if the `indices`s of `Rdest` and `Rsrc` are all `NTuple{N,<:AbstractUnitRange}`.

(cherry picked from commit a91be39)
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
There's no performance change, if the `indices`s of `Rdest` and `Rsrc` are all `NTuple{N,<:AbstractUnitRange}`.

(cherry picked from commit a91be39)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

copyto! with CartesianIndices assumes identical step in source and destination indices
4 participants