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

reshaped views and reinterpreted arrays #161

Merged
merged 6 commits into from
Jun 3, 2021

Conversation

ranocha
Copy link
Contributor

@ranocha ranocha commented Jun 2, 2021

I have fixed the issues reported in #157 and JuliaArrays/StaticArrayInterface.jl#2. Here, I concentrated on the exact applications using reshaped versions of views that are abstract vectors. In this case, perfect might be the enemy of good again: This solution isn't perfect but it solves the problems I have right now and can be extended later to reshaped arrays of more general array types. I also adapted some docstrings slightly to look more accurate to me.

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #161 (d84f2d4) into master (6a8efa6) will increase coverage by 0.06%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     JuliaArrays/ArrayInterface.jl#161      +/-   ##
==========================================
+ Coverage   82.74%   82.80%   +0.06%     
==========================================
  Files          11       11              
  Lines        1721     1733      +12     
==========================================
+ Hits         1424     1435      +11     
- Misses        297      298       +1     
Impacted Files Coverage Δ
src/stridelayout.jl 87.95% <92.85%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a8efa6...d84f2d4. Read the comment docs.

Copy link
Collaborator

@chriselrod chriselrod left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

Re "don't let perfect be the enemy of good", if you can't address the comment on reshaping PermutedDimsArray/transpose/adjoint at the moment, we can merge this now and file a new issue.

@@ -310,10 +310,11 @@ _is_column_major(sr::R, cbs::StaticInt) where {R} = False()
_is_column_major(sr::R, cbs::Union{StaticInt{0},StaticInt{-1}}) where {R} = is_increasing(sr)

"""
dense_dims(::Type{T}) -> NTuple{N,Bool}
dense_dims(::Type{<:AbstractArray{N}}) -> NTuple{N,Bool}
Copy link
Collaborator

Choose a reason for hiding this comment

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

NTuple{N,StaticBool}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Implemented in 7335b49

# TODO: Should be generalized to reshaped arrays wrapping more general array types
function strides(A::ReshapedArray{T,N,P}) where {T, N, P<:AbstractVector}
if defines_strides(A)
return size_to_strides(size(A), first(strides(parent(A))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about

julia> A = rand(5,12);

julia> B = reshape(A', (3,4,5));

ArrayInterface.strides(B) should be (5,15,StaticInt{1}()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to fix that later since I would need to think a bit more about it. I opened #162 to track this bug.

@ranocha ranocha requested a review from chriselrod June 2, 2021 14:32
@ranocha ranocha changed the title reshaped views reshaped views and reinterpreted arrays Jun 3, 2021
@ranocha
Copy link
Contributor Author

ranocha commented Jun 3, 2021

I pushed a fix for #163. I also updated the version in Project.toml since I would really like to have a new release to be able to use these fixes.

Copy link
Collaborator

@chriselrod chriselrod left a comment

Choose a reason for hiding this comment

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

LGTM.

@chriselrod chriselrod merged commit b91d803 into JuliaArrays:master Jun 3, 2021
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