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

Extend strides for ReshapedArray with strided parent. #44507

Merged
merged 4 commits into from
Jul 5, 2022

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Mar 7, 2022

#44027 defined strides(::ReshapedArray) for dense/stridedvector parents, where the dense check failed to cover inputs with size-1 dimension.

With this PR, strides is extended to all ReshapedArray whose parent has strides defined.
And the memory layout check for vector inputs before BLAS call is fixed by code reuse.

Close #44497.

@N5N3 N5N3 added the linear algebra Linear algebra label Mar 7, 2022
@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 8, 2022

This was an oversight (regression) in #44027, right, following the discussion after that PR was merged? That commit appears to be in v1.8.0 also, so this may need backporting

@vtjnash vtjnash added the backport 1.8 Change should be backported to release-1.8 label Mar 8, 2022
@N5N3 N5N3 added the arrays [a, r, r, a, y, s] label Mar 8, 2022
@N5N3 N5N3 force-pushed the ignore-sz1 branch 3 times, most recently from d07b00e to 7a081cb Compare March 11, 2022 04:06
@N5N3 N5N3 changed the title Ignore 1-sized dimension during vector layout check. Extend strides for ReshapedArray with strided parent. Mar 11, 2022
@KristofferC KristofferC mentioned this pull request Mar 11, 2022
47 tasks
@N5N3 N5N3 force-pushed the ignore-sz1 branch 2 times, most recently from 6f9f6ae to 9c9d28e Compare March 22, 2022 04:20
@jishnub
Copy link
Contributor

jishnub commented Apr 14, 2022

Is this PR ready to be merged? Or is there something holding it back?

@N5N3
Copy link
Member Author

N5N3 commented Apr 14, 2022

It's waiting for review. @timholy would you mind taking a quick look at it?

N5N3 added 2 commits May 25, 2022 09:21
Use `Base.merge_adjacent_dim` to perform vector layout check before BLAS call.

style clean
It turns out `strides(a::StridedReinterpretArray)` won't call `Base.size_to_strides` anymore after JuliaLang#44027.
As it is dispatched to `strides(::NonReshapedReinterpretArray)`/`strides(::ReshapedReinterpretArray)`.
This commit fix that regression.
Copy link
Sponsor Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I wasn't paying attention to Julia development for a couple of months due to other duties. This looks great, many thanks @N5N3!

@timholy
Copy link
Sponsor Member

timholy commented Jul 1, 2022

Let's run tests again since it's been a while.

@N5N3
Copy link
Member Author

N5N3 commented Jul 2, 2022

Never mind @timholy. CI looks busy st present, maybe we need to rerun the test when it's free.

@timholy timholy closed this Jul 2, 2022
@timholy timholy reopened this Jul 2, 2022
@timholy
Copy link
Sponsor Member

timholy commented Jul 5, 2022

I haven't been following CI lately, are these failures OK? If so would someone please merge?

@N5N3 N5N3 merged commit 0d3aca4 into JuliaLang:master Jul 5, 2022
@N5N3 N5N3 deleted the ignore-sz1 branch July 5, 2022 15:23
KristofferC pushed a commit that referenced this pull request Jul 6, 2022
* Extend `strides(::ReshapedArray)` with non-contiguous strided parent
* Make sure `strides(::StridedReinterpretArray)` calls `size_to_strides`

Co-authored-by: Tim Holy <tim.holy@gmail.com>
(cherry picked from commit 0d3aca4)
@KristofferC KristofferC mentioned this pull request Jul 6, 2022
15 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Jul 8, 2022
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
…44507)

* Extend `strides(::ReshapedArray)` with non-contiguous strided parent
* Make sure `strides(::StridedReinterpretArray)` calls `size_to_strides`

Co-authored-by: Tim Holy <tim.holy@gmail.com>
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
…44507)

* Extend `strides(::ReshapedArray)` with non-contiguous strided parent
* Make sure `strides(::StridedReinterpretArray)` calls `size_to_strides`

Co-authored-by: Tim Holy <tim.holy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

axpy! error:parent must be contiguous
5 participants