-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Document DenseArrays and Dims #28080
Conversation
base/array.jl
Outdated
|
||
One-dimensional dense array with elements of type `T`, often used to represent | ||
a mathematical vector. Alias for `DenseArray{T,1}`. The elements of a dense array | ||
are stored contiguously in memory, with no striding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get what you're trying to say here, but I think I'd refrain from using the phrase "no striding." A contiguous array still has strides
— it's just that they're arranged such that they cover the memory contiguously with no overlaps or skips.
Nice, thanks so much for tackling my massive list! How about just documenting |
Is |
Yup. It looks like my script in #26919 missed it — perhaps because it's a builtin |
Better? |
base/array.jl
Outdated
DenseVector{T} | ||
|
||
One-dimensional [`DenseArray`](@ref) with elements of type `T`, often used to represent | ||
a mathematical vector. Alias for `DenseArray{T,1}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "often used to represent a mathematical vector" part seems unnecessary here too.
base/array.jl
Outdated
DenseMatrix{T} | ||
|
||
Two-dimensional [`DenseArray`](@ref) with elements of type `T`, often used to represent | ||
a mathematical vector. Alias for `DenseArray{T,2}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here, and in any case change vector -> matrix.
""" | ||
Dims{N} | ||
|
||
An `NTuple` of `N` `Int`s used to represent the dimensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps change "An NTuple
of N
Int
s used to ..." to "An alias for NTuple{N,Int}
used to ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the second phrasing more opaque though? The current wording is more intuitive/decipherable for the uninitiated IMO.
base/array.jl
Outdated
DenseArray{T, N} <: AbstractArray{T,N} | ||
|
||
`N`-dimensional dense array with elements of type `T`, often used to represent | ||
a mathematical vector, matrix, or higher dimensional tensor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the
often used to represent a mathematical vector, matrix, or higher dimensional tensor.
part, these are used just as often for other things, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's useful to have those terms in the documentation to help with searching cross-referencing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but there's nothing special about DenseArray
that makes it particularly better at being a mathematical vector/matrix/tensor.
Test fails seem unrelated to me - anyone confirm? This good to go? |
Wow, that was a rough go on CI. The ccall failure in FreeBSD has been noted before. Mac was filewatching, Travis Linux 64 was the offset array failure (since fixed), and CircleCI has |
No description provided.