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 pointer to no longer assume contiguity #36405

Merged
merged 5 commits into from
Jun 26, 2020
Merged

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Jun 23, 2020

Fixes #36402. Turns out this affected both pointer(::SubArray) as well as pointer(::AbstractArray, i).

@mbauman mbauman added arrays [a, r, r, a, y, s] needs tests Unit tests are required for this change bugfix This change fixes an existing bug backport 1.5 labels Jun 23, 2020
@mbauman
Copy link
Member Author

mbauman commented Jun 24, 2020

As I went through this, I realized that there's a question mark in how pointer(A, i) is documented: does the i mean an offset of elements or does it mean an index?

help?> pointer
search: pointer pointer_from_objref unsafe_pointer_to_objref codepoint

  pointer(array [, index])

  Get the native address of an array or string, optionally at a given location index.

To me, location index reads like a getindexy index (and not a memory offset), but this was intentionally disabled by PermutedDimsArray because it wasn't clear. This would definitively make it an index-location.

# It's OK to return a pointer to the first element, and indeed quite
# useful for wrapping C routines that require a different storage
# order than used by Julia. But for an array with unconventional
# storage order, a linear offset is ambiguous---is it a memory offset
# or a linear index?
Base.pointer(A::PermutedDimsArray, i::Integer) = throw(ArgumentError("pointer(A, i) is deliberately unsupported for PermutedDimsArray"))

I'm really just trying to fix #36402 in this PR (and intentionally structured this to allow for further improvements to pointer, and we could re-enable PermutedDimsArray later, too), but I just wanted to call this ambiguity out. Cc @timholy and #20385.

@vtjnash
Copy link
Member

vtjnash commented Jun 24, 2020

It's intended to be the index of the ith element (getindexy)

@codecov-commenter
Copy link

Codecov Report

Merging #36405 into master will increase coverage by 0.00%.
The diff coverage is 83.90%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #36405   +/-   ##
=======================================
  Coverage   86.13%   86.14%           
=======================================
  Files         349      349           
  Lines       65323    65401   +78     
=======================================
+ Hits        56265    56337   +72     
- Misses       9058     9064    +6     
Impacted Files Coverage Δ
base/abstractarraymath.jl 94.81% <ø> (ø)
base/file.jl 79.00% <ø> (ø)
base/math.jl 90.57% <ø> (ø)
base/special/trig.jl 94.36% <79.10%> (-1.99%) ⬇️
base/abstractarray.jl 86.36% <100.00%> (+0.05%) ⬆️
base/array.jl 95.55% <100.00%> (ø)
base/methodshow.jl 82.25% <100.00%> (ø)
base/reflection.jl 86.92% <100.00%> (+0.36%) ⬆️
stdlib/LinearAlgebra/src/givens.jl 56.73% <0.00%> (-4.81%) ⬇️
stdlib/SparseArrays/src/linalg.jl 95.24% <0.00%> (+0.19%) ⬆️
... and 4 more

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 8db9495...65f4046. Read the comment docs.

@mbauman mbauman force-pushed the mb/restridingsubarray branch from 65f4046 to 5485207 Compare June 25, 2020 04:58
@KristofferC KristofferC removed the needs tests Unit tests are required for this change label Jun 25, 2020
@KristofferC
Copy link
Member

Good to go?

@KristofferC KristofferC mentioned this pull request Jun 25, 2020
9 tasks
@mbauman
Copy link
Member Author

mbauman commented Jun 25, 2020

Yes, this is good to go on my side once tests pass.

@KristofferC KristofferC merged commit 59b8dde into master Jun 26, 2020
@KristofferC KristofferC deleted the mb/restridingsubarray branch June 26, 2020 09:21
KristofferC pushed a commit that referenced this pull request Jun 26, 2020
* Fix pointer to no longer assume contiguity

(cherry picked from commit 59b8dde)
mbauman added a commit to dlfivefifty/julia that referenced this pull request Jun 26, 2020
* 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 added a commit that referenced this pull request Jul 21, 2020
Followup to #36739 (and currently built atop it), this restores the `pointer(::SubArray{<:Any,<:Any,<:Array,<:Tuple{Vararg{RangeIndex}}}, ::Tuple)` method that was removed in #36405. It does so, however, as a deprecated method, with the actual method being implemented on `(::SubArray{...}, ::CartesianIndex)`.

The rationale here is because the `::Tuple` method was undocumented and only supported on that one _highly_ specific `SubArray` type. I am keeping this patch as minimal as possible for backporting; in the future I aim to support `Vararg{CartesianIndex, Integer}` locations to make this more `getindex`-y and move farther away from the conflation with memory offsets.
JeffBezanson pushed a commit that referenced this pull request Jul 22, 2020
Followup to #36739 (and currently built atop it), this restores the `pointer(::SubArray{<:Any,<:Any,<:Array,<:Tuple{Vararg{RangeIndex}}}, ::Tuple)` method that was removed in #36405. It does so, however, as a deprecated method, with the actual method being implemented on `(::SubArray{...}, ::CartesianIndex)`.

The rationale here is because the `::Tuple` method was undocumented and only supported on that one _highly_ specific `SubArray` type. I am keeping this patch as minimal as possible for backporting; in the future I aim to support `Vararg{CartesianIndex, Integer}` locations to make this more `getindex`-y and move farther away from the conflation with memory offsets.
JeffBezanson pushed a commit that referenced this pull request Jul 23, 2020
Followup to #36739 (and currently built atop it), this restores the `pointer(::SubArray{<:Any,<:Any,<:Array,<:Tuple{Vararg{RangeIndex}}}, ::Tuple)` method that was removed in #36405. It does so, however, as a deprecated method, with the actual method being implemented on `(::SubArray{...}, ::CartesianIndex)`.

The rationale here is because the `::Tuple` method was undocumented and only supported on that one _highly_ specific `SubArray` type. I am keeping this patch as minimal as possible for backporting; in the future I aim to support `Vararg{CartesianIndex, Integer}` locations to make this more `getindex`-y and move farther away from the conflation with memory offsets.
JeffBezanson pushed a commit that referenced this pull request Jul 23, 2020
Followup to #36739 (and currently built atop it), this restores the `pointer(::SubArray{<:Any,<:Any,<:Array,<:Tuple{Vararg{RangeIndex}}}, ::Tuple)` method that was removed in #36405. It does so, however, as a deprecated method, with the actual method being implemented on `(::SubArray{...}, ::CartesianIndex)`.

The rationale here is because the `::Tuple` method was undocumented and only supported on that one _highly_ specific `SubArray` type. I am keeping this patch as minimal as possible for backporting; in the future I aim to support `Vararg{CartesianIndex, Integer}` locations to make this more `getindex`-y and move farther away from the conflation with memory offsets.

(cherry picked from commit d9b7d7e)
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
* Fix pointer to no longer assume contiguity
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
…rt (JuliaLang#36757)

Followup to JuliaLang#36739 (and currently built atop it), this restores the `pointer(::SubArray{<:Any,<:Any,<:Array,<:Tuple{Vararg{RangeIndex}}}, ::Tuple)` method that was removed in JuliaLang#36405. It does so, however, as a deprecated method, with the actual method being implemented on `(::SubArray{...}, ::CartesianIndex)`.

The rationale here is because the `::Tuple` method was undocumented and only supported on that one _highly_ specific `SubArray` type. I am keeping this patch as minimal as possible for backporting; in the future I aim to support `Vararg{CartesianIndex, Integer}` locations to make this more `getindex`-y and move farther away from the conflation with memory offsets.
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] bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.5: Surprising new pointer behaviour with adjoints and views
4 participants