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

Remove unnecessary restriction to StridedVecOrMat #35929

Merged
merged 14 commits into from
Jun 30, 2020

Conversation

dlfivefifty
Copy link
Contributor

@dlfivefifty dlfivefifty commented May 18, 2020

The "Strided array interface" https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-strided-arrays-1 means that this is useful beyond just StridedVecOrMat so there is no reason to be so restrictive.

@dlfivefifty
Copy link
Contributor Author

Note ArrayLayouts.jl also has an approach for pointers to adjoints of complex arrays by defining ConjPtr so that, for example, pointer(view(A',1:3,2:4)') works. This could be moved to Base and thereby removing type piracy for ArrayLayouts.jl:

"""
    ConjPtr{T}
represents that the entry is the complex-conjugate of the pointed to entry.
"""
struct ConjPtr{T} 
    ptr::Ptr{T}
end

unsafe_convert(::Type{ConjPtr{T}}, Ac::Adjoint{<:Complex}) where T<:Complex = unsafe_convert(Ptr{T}, parent(Ac))    
function unsafe_convert(::Type{ConjPtr{T}}, V::SubArray{T,2}) where {T,N,P}
    kr, jr = parentindices(V)
    unsafe_convert(Ptr{T}, view(parent(V)', jr, kr))
end

@mbauman mbauman added arrays [a, r, r, a, y, s] needs tests Unit tests are required for this change labels May 18, 2020
@dlfivefifty
Copy link
Contributor Author

Can this get merged so I can start testing all the packages I manage on Julia v1.5?

@KristofferC
Copy link
Member

It seems a test was requested.

@dlfivefifty
Copy link
Contributor Author

I'm a bit confused by the following:

julia> strides(randn(4)')
(4, 1)

julia> strides([1 2 3 4])
(1, 1)

Shouldn't strides(randn(4)') return (1,1)?

@mbauman
Copy link
Member

mbauman commented Jun 5, 2020

Good question... but is there a case in which it matters? You're not allowed to multiply that first value by anything other than zero, are you?

Is there a way in which (1, 1) is any more correct or helpful than (4, 1)? I suppose it's easier to flag (1,1) as contiguous.

@dlfivefifty
Copy link
Contributor Author

dlfivefifty commented Jun 5, 2020

Only because stride(::AbstractVector, 2) throws an error at the moment. I changed the definition to work around this but if you really want (4,1) we can think of an alternative.

@mbauman
Copy link
Member

mbauman commented Jun 5, 2020

This is my main motivation:

julia> strides(randn(4, 1)')
(4, 1)

julia> strides(randn(4)')
(4, 1)

@KristofferC KristofferC added this to the 1.5 milestone Jun 5, 2020
@dlfivefifty
Copy link
Contributor Author

OK that's a strong argument. So I generalised stride(::AbstractArray, k::Integer) to be defined for all k, as in Array.

I also added ConjPtr to deal with pointers of complex adjoints. This could be removed if you aren't happy with it (though would mean ArrayLayouts.jl would still have some type piracy).

@mbauman Could you review the changes again please?

@dlfivefifty
Copy link
Contributor Author

I believe this is ready to merge now unless you want other changes made.

base/abstractarray.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Member

Whitespace needs fixing. If this is not merged soon, I'll just push for a 1.5-RC1 since the only thing this "fixes" is a package that was doing type piracy anyway and could just be worked around in the package.

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
@mbauman mbauman removed the needs tests Unit tests are required for this change label Jun 9, 2020
@dlfivefifty
Copy link
Contributor Author

The test failure looks unrelated

@dlfivefifty
Copy link
Contributor Author

Does "backport-1.5" mean it's not going to be in v1.5.0? If so I'll start working around the issue in ArrayLayouts.jl for v1.5-beta

@KristofferC
Copy link
Member

Does "backport-1.5" mean it's not going to be in v1.5.0?

No, if it gets merged before 1.5.0 is released, then it will be in.

mbauman added 3 commits June 26, 2020 08:25
* origin/master: (232 commits)
  Add passthrough for non-Markdown docs (JuliaLang#36091)
  Fix pointer to no longer assume contiguity (JuliaLang#36405)
  Ensure string-hashing is defined before it gets used (JuliaLang#36411)
  Make compilecache atomic (JuliaLang#36416)
  add a test for JuliaLang#30739 (JuliaLang#36395)
  Fix broken links in docstring of `repeat` (JuliaLang#36376)
  fix and de-dup cached calls to `methods_by_ftype` in compiler (JuliaLang#36404)
  ml-matches: skip unnecessary work, when possible (JuliaLang#36413)
  gf: fix some issues with the move from using a tree to a hash lookup of leaf types (JuliaLang#36413)
  Add news and manual entry for sincospi (JuliaLang#36403)
  Check axes in Array(::AbstractArray) (fixes JuliaLang#36220) (JuliaLang#36397)
  add versions of `code_typed` and `which` that accept tuple types (JuliaLang#36389)
  Fix spelling of readdir. (JuliaLang#36409)
  add sincospi (JuliaLang#35816)
  fix showing methods with unicode gensymed variable names (JuliaLang#36396)
  Add doctest: eachslice (JuliaLang#36386)
  fix documentation typo ("Ingeger")
  Refactor `abstract_eval` to separate out statements and values (JuliaLang#36350)
  fix return type of `get!` on `IdDict` (JuliaLang#36383)
  Allow single option with REPL.TerminalMenus (JuliaLang#36369)
  ...
@mbauman mbauman added the merge me PR is reviewed. Merge when all tests are passing label Jun 26, 2020
@mbauman mbauman merged commit 6b2c7f1 into JuliaLang:master Jun 30, 2020
@mbauman mbauman removed the merge me PR is reviewed. Merge when all tests are passing label Jun 30, 2020
KristofferC pushed a commit that referenced this pull request Jul 8, 2020
* Remove unnecessary restriction to `StridedVecOrMat`

The "Strided array interface" https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-strided-arrays-1 means that this is useful beyond these types

* Update adjtrans.jl

* Add tests for adj/trans strides

* Add tests, change strides(::Adjoint{<:Any,<:AbstractVector}) definition

* stride(::AbstractrArray, k) for all k, add ConjPtr

* Remove ConjPtr

* Always throw an error if strides is not implemented

* Update abstractarray.jl

* Update blas.jl

* Remove k < 1 special case

* Also widen elsize to AbstractVecOrMat

* Use strides for dim > ndims

* Update stdlib/LinearAlgebra/test/blas.jl

Co-authored-by: Matt Bauman <[email protected]>
(cherry picked from commit 6b2c7f1)
@KristofferC KristofferC mentioned this pull request Jul 8, 2020
13 tasks
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
* Remove unnecessary restriction to `StridedVecOrMat`

The "Strided array interface" https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-strided-arrays-1 means that this is useful beyond these types

* Update adjtrans.jl

* Add tests for adj/trans strides

* Add tests, change strides(::Adjoint{<:Any,<:AbstractVector}) definition

* stride(::AbstractrArray, k) for all k, add ConjPtr

* Remove ConjPtr

* Always throw an error if strides is not implemented

* Update abstractarray.jl

* Update blas.jl

* Remove k < 1 special case

* Also widen elsize to AbstractVecOrMat

* Use strides for dim > ndims

* Update stdlib/LinearAlgebra/test/blas.jl

Co-authored-by: Matt Bauman <[email protected]>
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]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants