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

Document DenseArrays and Dims #28080

Merged
merged 1 commit into from
Jul 14, 2018
Merged

Document DenseArrays and Dims #28080

merged 1 commit into from
Jul 14, 2018

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Jul 12, 2018

No description provided.

@kshyatt kshyatt added domain:docs This change adds or pertains to documentation domain:arrays [a, r, r, a, y, s] labels Jul 12, 2018
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.
Copy link
Sponsor Member

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.

@mbauman
Copy link
Sponsor Member

mbauman commented Jul 12, 2018

Nice, thanks so much for tackling my massive list!

How about just documenting DenseArray with the meaty stuff once and then having DenseVector and DenseMatrix just be pointers to it — like you've already done for DenseVecOrMat?

@kshyatt
Copy link
Contributor Author

kshyatt commented Jul 12, 2018

Is DenseArray exported?

@mbauman
Copy link
Sponsor Member

mbauman commented Jul 12, 2018

Yup. It looks like my script in #26919 missed it — perhaps because it's a builtin Core binding.

@kshyatt
Copy link
Contributor Author

kshyatt commented Jul 12, 2018

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}`.
Copy link
Member

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}`.
Copy link
Member

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
Copy link
Member

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 Ints used to ..." to "An alias for NTuple{N,Int} used to ..."

Copy link
Contributor

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.
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Sponsor Member

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.

@kshyatt
Copy link
Contributor Author

kshyatt commented Jul 14, 2018

Test fails seem unrelated to me - anyone confirm? This good to go?

@mbauman
Copy link
Sponsor Member

mbauman commented Jul 14, 2018

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 JULIA_CPU_CORES in its config.

@mbauman mbauman merged commit faee1df into master Jul 14, 2018
@mbauman mbauman deleted the ksh/densedims branch July 14, 2018 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants