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

Array docs: Clarify DenseArray vs. "strided" #26085

Merged
merged 3 commits into from
Feb 20, 2018
Merged

Array docs: Clarify DenseArray vs. "strided" #26085

merged 3 commits into from
Feb 20, 2018

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Feb 16, 2018

Dense Arrays absolutely must be contiguously arranged in memory. This attempts to make documentation match the current reality.

Dense Arrays absolutely must be contiguously arranged in memory.  This attempts to make documentation match the current reality.
@mbauman mbauman added docs This change adds or pertains to documentation arrays [a, r, r, a, y, s] labels Feb 16, 2018
@mbauman
Copy link
Member Author

mbauman commented Feb 16, 2018

Cc: @dlfivefifty — since you wrote the strided array interface section I think it'd be nice to get your eyes on this.

to call a wider range of BLAS and LAPACK functions by passing them either [`Array`](@ref) or
`SubArray` objects, and thus saving inefficiencies from memory allocation and copying.
A "strided" array is stored in memory and has its elements are laid out in regular offsets such that
it can be passed to external C and Fortran functions that expect this memory layout. Strided arrays
Copy link
Member

Choose a reason for hiding this comment

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

... such that an instance with an isbitstype eltype can be passed ...?

@dlfivefifty
Copy link
Contributor

I think it should be renamed DenseColumnArray or ColumnMajorArray to clarify the new usage.

@mbauman
Copy link
Member Author

mbauman commented Feb 17, 2018

Yeah, we can tackle a renaming separately. This is just aiming to get the documentation correct.

We can tackle more general definitions later.
`StridedVector` and `StridedMatrix` are convenient aliases defined to make it possible for Julia
to call a wider range of BLAS and LAPACK functions by passing them either [`Array`](@ref) or
`SubArray` objects, and thus saving inefficiencies from memory allocation and copying.
A "strided" array is stored in memory and has its elements are laid out in regular offsets such that
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be either "and has its elements laid out" or "and its elements are laid out".

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thank you!

@dlfivefifty
Copy link
Contributor

The reason I think it should be renamed is that it actually changes the meaning of DenseArray. Though I only know of two subtypes so it’s not that big a deal.

@mbauman mbauman merged commit 5aa548b into master Feb 20, 2018
@mbauman mbauman deleted the mbauman-patch-1 branch February 20, 2018 14:42
must define a [`strides(A)`](@ref) method that returns a tuple of "strides" for each dimension; a
provided [`stride(A,k)`](@ref) method accesses the `k`th element within this tuple. Increasing the
index of dimension `k` by `1` should increase the index `i` of [`getindex(A,i)`](@ref) by
[`stride(A,k)`](@ref). If a pointer conversion method [`Base.unsafe_convert(Ptr{T}, A)`](@ref) is
Copy link
Contributor

@Jutho Jutho Feb 21, 2018

Choose a reason for hiding this comment

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

Is this true? Is getindex(A,i) a way to directly access elements based on offsets from stride calculations, or is it really still linear indexing that assumes getindex(A,i_1,i_2,...) == getindex(A, i_1 + size(A,1)*(i_2-1) + ...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, you're right, thank you. getindex is totally and completely decoupled from strides.

@stevengj
Copy link
Member

I just came across this "clarification" PR. I agree with @dlfivefifty that it is a change in meaning rather than a clarification. The previous meaning (which was initially slated to be called RegularArray, but it was agreed at the time to keep the name DenseArray) was discussed in #6258. Now we have no abstract supertype for strided arrays. 😞

The new meaning still works for all the DenseArray subtypes in base, of course. Hopefully it didn't invalidate any code in packages. Anyway, water under the bridge at this point.

@mbauman
Copy link
Member Author

mbauman commented Feb 28, 2024

I’m not sure which change started changing the meaning here, but it wasn’t this one.

I believe the problem was that all the Base DenseArrays happened to be contiguous, and so it just gradually became the expectation.

@mbauman
Copy link
Member Author

mbauman commented Feb 28, 2024

Oh, yeah, this actually started with me trying to reclaim the meaning of DenseArray to mean strided: #26013

There are lots of things you can drag me for from 6 years ago — I have plenty of other regrets that my own brain won't let me forget. I don't think this one needs to be added to that pile.

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] docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants