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

Allow negative stride(A,2) in gemv! #42054

Merged
merged 2 commits into from
Feb 9, 2022
Merged

Conversation

sostock
Copy link
Contributor

@sostock sostock commented Aug 30, 2021

This replaces #42012 as another attempt at fixing #41513 (comment). It implements the idea from #41513 (comment).

Instead of just allowing stride(A,2) < 1 in trivial cases, the requirement stride(A,2) ≥ max(1, size(A,1)) is relaxed to abs(stride(A,2)) ≥ size(A,1) || size(A,2) ≤ 1.

We could also think about relaxing the requirement stride(A,1) == 1 to abs(stride(A,1)) == 1.

Closes #42012.

@dkarrasch dkarrasch added the domain:linear algebra Linear algebra label Aug 30, 2021
@sostock sostock added the kind:regression Regression in behavior compared to a previous version label Oct 24, 2021
@sostock
Copy link
Contributor Author

sostock commented Feb 1, 2022

Since #41513 is now marked for backport, this PR (which I prefer over #42012) should be backported as well (as discussed on discourse).

Closing and re-opening to run the tests against current master.

@sostock sostock closed this Feb 1, 2022
@sostock sostock reopened this Feb 1, 2022
@sostock sostock added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Feb 1, 2022
@dkarrasch
Copy link
Member

IIUC you need to rebase manually. Closing and re-opening does not rebase onto current master. I think we should get this one merged (and then backported), and then put #42957 on top.

@N5N3 would you be available to take a quick look?

@N5N3
Copy link
Member

N5N3 commented Feb 8, 2022

LGTM, the only (little) improvement I can found is simplify the pointer calculation of A (L679).

@dkarrasch
Copy link
Member

LGTM, the only (little) improvement I can found is simplify the pointer calculation of A (L679).

Thanks for the speedy response. Since this PR is supposed to be backported, I don't think we should mix things here, but keep it as is, backport, and develop further on master/v1.8 in your PR.

@dkarrasch
Copy link
Member

I believe the test failures are nothing but internet connection failure at the time. I'll rerun and merge once tests (essentially) pass.

@dkarrasch dkarrasch closed this Feb 8, 2022
@dkarrasch dkarrasch reopened this Feb 8, 2022
@dkarrasch dkarrasch added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 8, 2022
@sostock
Copy link
Contributor Author

sostock commented Feb 8, 2022

I just rebased onto the current master. The LinearAlgebra/blas tests pass on my machine.

@dkarrasch dkarrasch merged commit 33a71b7 into JuliaLang:master Feb 9, 2022
@DilumAluthge DilumAluthge removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 9, 2022
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request Feb 17, 2022
@KristofferC KristofferC mentioned this pull request Feb 19, 2022
50 tasks
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@sostock sostock mentioned this pull request Feb 23, 2022
40 tasks
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
KristofferC pushed a commit that referenced this pull request Mar 15, 2022
(cherry picked from commit 33a71b7)
@KristofferC
Copy link
Sponsor Member

This causes a bunch of packages to fail on 1.6: https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/c7daef8_vs_7be0dcd/report.html. So I will drop this from the 1.6 backports.

@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Mar 18, 2022
@sostock
Copy link
Contributor Author

sostock commented Mar 19, 2022

Backporting #41513 but not either this or #42012 might also break things (see #41513 (comment)). I will investigate what happens here.

Maybe we could backport #42012 instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:linear algebra Linear algebra kind:regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants