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

Strides for Adjoint & Transpose #710

Merged
merged 4 commits into from
Jul 15, 2020
Merged

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented Jul 1, 2020

This adds JuliaLang/julia#35929 which updated JuliaLang/julia#29135 .

Edit: the more recently merged PR JuliaLang/julia#35929 is marked "backport 1.5". I presume that this means the VERSION cutoff used here may need to change.

@martinholters
Copy link
Member

I'll review later, but on first glance: Needs a README entry.

@mcabbott
Copy link
Contributor Author

mcabbott commented Jul 1, 2020

Sure. If I do it after #709 then I won't have to fix clashes.

This one may need to wait for the backport anyway.

src/Compat.jl Outdated Show resolved Hide resolved
@martinholters
Copy link
Member

LGTM. Once the backport is done and the version conditional updated accordingly, this should be good to go.

src/Compat.jl Outdated Show resolved Hide resolved
Co-authored-by: Martin Holters <martin.holters@hsu-hh.de>
@martinholters
Copy link
Member

Travis Linux Julia 1.0:

generalized dot #32739: Test Failed at /home/travis/build/JuliaLang/Compat.jl/test/runtests.jl:173

  Expression: dot(x, transpose(A), y) ≈ dot(x, transpose(A) * y) ≈ x' * transpose(A) * y ≈ (x' * transpose(A)) * y

   Evaluated: 0.0016273856f0 ≈ 0.0016267318f0 ≈ 0.0016271947f0 ≈ 0.0016271947f0

AV Julia latest (1.6.0-DEV.452) 32 bit:

generalized dot #32739: Test Failed at C:\projects\compat-jl\test\runtests.jl:171
  Expression: dot(x, A, y) ≈ dot(A' * x, y) ≈ x' * A * y ≈ (x' * A) * y
   Evaluated: 0.0003067404f0 ≈ 0.00030654308f0 ≈ 0.00030654308f0 ≈ 0.00030654308f0

All others pass. Does this influence the dispatch of dot in some way here? E.g., going through BLAS more often than before or something?

The inputs here are randomly generated, that might explain why it only fails sometimes, but I haven't seen this before, so most likely related to this PR.

@martinholters
Copy link
Member

On second thought, this PR shouldn't change anything on Julia 1.6.0-DEV.452 (except for adding tests, which are not the ones failing here), so hard to blame it. But the same change in Base could lead to same problem in the here, of course.

@mcabbott
Copy link
Contributor Author

I don't understand this at all. It's both 1.0 on Travis and master on Appveyor, on the same test. Both seem like they ought to have been identical on earlier runs of this PR.

And I can't see how strides would change dot, rand(2,2)' isa StridedArray remains false so it shouldn't dispatch differently.

@martinholters
Copy link
Member

Running

@testset begin
    n = 10
    for iter in 1:1000000
        A = randn(Float32, n, n)
        x = rand(Float32, n)
        y = rand(Float32, n)
        @test dot(x, transpose(A), y)  dot(x, transpose(A)*y)  *(x', transpose(A), y)  (x'*transpose(A))*y
    end
end

gives around 200 fails across different Julia versions. Seems we were just very unlucky with the latest CI run. I'll restart and if it looks better this time, this should be ready to merge. The flaky test is an orthogonal issue.

@martinholters martinholters reopened this Jul 15, 2020
@martinholters martinholters merged commit c3984f3 into JuliaLang:master Jul 15, 2020
@mcabbott mcabbott deleted the strides branch July 15, 2020 16:05
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.

2 participants