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

RFC: Add convert methods from iterables to array/vector/matrix #16633

Closed
wants to merge 1 commit into from

Conversation

nalimilan
Copy link
Member

These allow using Vector(itr) or Array(itr) as a replacement for
collect(). Though two significant exceptions only work with the convert()
syntax: integers and tuples, for which special constructors already exist
to create uninitialized arrays.


This is a first step towards #16029. I think this PR makes sense on its own, despite from the obvious issue regarding integers and tuples, which makes deprecating collect hard at this point.

Apart from this, I'd like to hear your opinions about what to do in case of dimension mismatch: should we allow converting between collections of different number of dimensions? For now, Vector(itr) calls vec using the column-major convention. But for higher rank (i.e. Array{T, N} when N>2 is specified), an error is raised if the number of dimensions do not match. I think a reasonable behavior would be to drop trailing singleton dimensions (squeeze) or add missing dimensions at the end (no function for this?). Does that make sense to you?

A notable special case of this are scalars: ATM, the convert(Array, 1) gives an Array{Int,0}, and convert(Vector, 1) gives a Vector{Int}. For higher rank, an error is printed, in line with the previous point.


The syntax `Array(T, dims)` is also available, but deprecated.


Copy link
Member Author

Choose a reason for hiding this comment

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

Splitting this docstring in two, both associated to Array, gives an error at build time:

docs/Docs.jl
error during bootstrap:
LoadError(at "sysimg.jl" line 342: UndefVarError(var=:STDERR))

Yet, doing the same at the REPL works fine.

@MichaelHatherly Is that expected?

Copy link
Member

Choose a reason for hiding this comment

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

Trying to associate two docstrings with the same signature should print a warning and only store the second. I suppose you're getting the undefined error there since during bootstrap it isn't defined. I'll fix that up tomorrow.

Best to attach each one to a more specific signature if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Yet, running `"a" Array" at the REPL adds "a" to the existing docstring.

Indeed I would like to attach the message to a more specific signature, but these are defined as methods for convert, and users are more likely to look for them under Array. If every type adds docstrings to convert, its help page is going to be unusable pretty soon.

That convert fallback stuff might be more confusing than useful...

Copy link
Member

Choose a reason for hiding this comment

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

Defining a new docstring at the REPL would add it to Main's docstring cache and then using ? will concatenate docs from all modules, whereas defining extra ones in that file would add them to Base's cache. Duplicates are only checked for within the current module. Maybe a more thorough check could be done, though that might get a bit expensive perhaps.

Perhaps the following would work:

" ... "
Array(::Tuple), Vector(::Integer), Matrix(::Integer, ::Integer)

" ... "
Array(itr), Vector(itr), Matrix(itr)

or something similar to that.

Appending to convert's docs is probably a bad idea :)

These allow using Vector(itr) or Array(itr) as a replacement for
collect(). Though two significant exceptions only work with the convert()
syntax: integers and tuples, for which special constructors already exist
to create uninitialized arrays.
@nalimilan
Copy link
Member Author

Funny fact I just discovered due to a test failure:

julia> collect(Int, "a")
1-element Array{Int64,1}:
 97

I guess that's perfectly logical, though certainly surprising at first.

@@ -151,7 +151,7 @@ end
let
d = Dict{String, Vector{Int}}()
d["a"] = [1, 2]
@test_throws MethodError d["b"] = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a bit unfortunate

Copy link
Member Author

@nalimilan nalimilan May 30, 2016

Choose a reason for hiding this comment

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

This comes from the fact that scalars are iterable ATM. We could get rid of these two behaviours at the same time.

@nalimilan nalimilan changed the title Add convert methods from iterables to array/vector/matrix RFC: Add convert methods from iterables to array/vector/matrix May 30, 2016
@mschauer
Copy link
Contributor

mschauer commented Jun 3, 2016

Maybe it would be best to let convert fail in any case when there is a dimension mismatch for now: that avoids to introduce an instance where Array{T,1} behaves different from Array{T,N} and solves problem with numbers in the discussion, because numbers have size(...) = ( ) and can under this convention only be converted to zero-dimensional arrays in setindex! .

@nalimilan
Copy link
Member Author

@mschauer The behavior of collect in 0.4 is to always return a Vector, that's why I followed that convention. Though in 0.5, collect now respects the dimensionality of its argument: if that's considered OK we could certainly throw a DimensionMismatch from convert.

@JeffBezanson
Copy link
Member

The existing convention is to use constructors for this, not convert:

julia> convert(Set, [1,2,3])
ERROR: MethodError: Cannot `convert` an object of type Array{Int64,1} to an object of type Set{T}

julia> Set([1,2,3])
Set([2,3,1])

The ambiguity with tuple arguments will also need to be addressed.

@JeffBezanson
Copy link
Member

I don't think these should be methods of convert.

@tkelman tkelman deleted the nl/collect branch August 11, 2017 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants