search

davidavdav / NamedArrays.jl

Julia type that implements a drop-in replacement of Array with named dimensions
Other
120 stars 20 forks source link

Change more constructors to accept AbstractVectors #85

Closed nalimilan closed 4 years ago

nalimilan commented 4 years ago

Also add tests.

Closes #84.

nalimilan commented 4 years ago

Thanks! Would it be possible to tag a new release?

davidavdav commented 4 years ago

There seems to be a strange interaction with #86, which happened to be merged first, where one of the tests you added fails...

davidavdav commented 4 years ago

@nalimilan it looks like that if we want to allow for #86, the tests you added have to be reformulated as

n2 = @inferred NamedArray([1], BitArray([true]))
n3 = @inferred NamedArray([1], [true])

i.e., reduce the depth of arrays-of-arrays. The specification using tuple-of-arrays still is valid, as I consider this a more basic constructor

n1 = @inferred NamedArray([1], (BitArray([true]),))

So this way the use of type Array for the names argument really is a convenience method.

Would this be compatible with your intended use (you would probably want to use the constructor using a tuple from your code)?

nalimilan commented 4 years ago

Ah, too bad! Yes, I'm using the tuple constructor so any test is fine for me.

But generally speaking it seems to me that having two constructors accepting arrays but interpreting them differently is a recipe for problems. How about deprecating the one taking a vector of vectors in favor of the one taking a tuple of vectors? Anyway, tuples are a good choice when their length corresponds to the number of dimensions of an array, as it is a type parameter (like the length of tuples).

Otherwise, it would at least be useful to restrict the signature of this method so that names::AbstractVector{<:AbstractVector}: https://github.com/davidavdav/NamedArrays.jl/commit/9d8a687d775a5900184799b9134339e14e6cfcb9#diff-07f2a2b7c663cbb0d4a315dd954d510cR54

davidavdav commented 4 years ago

OK thanks for your wise words. I'll have a go at deprication of the Array convenience methods