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 dispatch for rdiv! with LU #55764

Merged
merged 4 commits into from
Nov 1, 2024
Merged

Fix dispatch for rdiv! with LU #55764

merged 4 commits into from
Nov 1, 2024

Conversation

dkarrasch
Copy link
Member

This method (added in #31853) was more specific than the one on line 493, which was added in #31285 at about the same time.

Fixes #55422.

@dkarrasch dkarrasch added performance Must go faster linear algebra Linear algebra backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Sep 13, 2024
@dkarrasch
Copy link
Member Author

This will need to be backported manually...

@dkarrasch dkarrasch removed backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Sep 13, 2024
@dkarrasch
Copy link
Member Author

For the reproducer in the original issue, I get

julia> A = rand(1000,1000); @btime inv($A); @btime inv2($A); @btime inv3($A);
  25.634 ms (10 allocations: 8.13 MiB)
  38.917 ms (9 allocations: 15.27 MiB)
  37.542 ms (9 allocations: 15.27 MiB)

julia> inv(A)  inv2(A)  inv3(A)
true

@dkarrasch
Copy link
Member Author

@jishnub The failures are unrelated, but still concerning. Do you remember if those NaN-ignoring things were known and maybe fixed? Since these tests are random, they may become invisible in another CI run, but we need to handle them, right?

@jishnub
Copy link
Contributor

jishnub commented Oct 27, 2024

I think these were fixed in #55737 and #55777.

The best way to check would be to rebase and rerun the tests locally by specifying the same random seed that had caused the failures, as suggested in #55727 (comment)

@dkarrasch
Copy link
Member Author

How do I extract that seed from "my" previous run? Is it printed somewhere in the test logs? Oh, or is it the "global RNG seed" printed after the big table?

@jishnub
Copy link
Contributor

jishnub commented Oct 27, 2024

Yes, that's the seed

@dkarrasch
Copy link
Member Author

Okay, unfortunately it failed only on Mac, and not on linux, so I can't test it locally. Anyway, let's merge this when tests pass, and assume this was nothing new, but the fixed issue.

@jishnub
Copy link
Contributor

jishnub commented Oct 27, 2024

But the tests on Linux and Mac must have used different seeds, no? Tests for a specific seed would fall on all platforms if it's due to the issue above. In any case, I think it's fixed now.

@dkarrasch dkarrasch merged commit 4393f8c into master Nov 1, 2024
5 of 7 checks passed
@dkarrasch dkarrasch deleted the dk/rdiv_lu branch November 1, 2024 20:23
@dkarrasch dkarrasch added the backport 1.11 Change should be backported to release-1.11 label Nov 1, 2024
dkarrasch added a commit that referenced this pull request Nov 1, 2024
@dkarrasch dkarrasch mentioned this pull request Nov 1, 2024
43 tasks
dkarrasch added a commit that referenced this pull request Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.11 Change should be backported to release-1.11 linear algebra Linear algebra performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rdiv! on LU object is much slower than ldiv!
2 participants