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

Add DataFrame constructors allowing NTuple and collection of Pair-s #1717

Merged
merged 12 commits into from
Feb 20, 2019

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Feb 11, 2019

Following #1599 I have added 3 new constructors (allowing a Tuple for columns and for names - for completeness).

@bkamins bkamins mentioned this pull request Feb 11, 2019
@bkamins bkamins changed the title Add DataFrame constructors allowing NTuple Add DataFrame constructors allowing NTuple and collection of Pair-s Feb 13, 2019
@bkamins
Copy link
Member Author

bkamins commented Feb 13, 2019

I have added a constructor with collection of pairs also (without a deprecation - but I can add it if we found we need to).

@bkamins
Copy link
Member Author

bkamins commented Feb 13, 2019

@nalimilan this should be good to have a look at. No rush.

@pdeffebach
Copy link
Contributor

Is it necessary to have DataFrames constructors like this? Why not just off-load everything to Tables.jl?

@bkamins
Copy link
Member Author

bkamins commented Feb 13, 2019

  1. constructor with iterable of Pairs produces a weird result (see DataFrame constructors #1599 for an example) because of Tables.jl and is inconsistent with splatting the iterator
  2. The other three constructors with Tuples now throw an error and are completely incompatibile to Tables.jl; we probably can live without them, they are added to complement DataFrame(Vector,Vector) constructor as in setindex! discussions (WIP: implementation of new setindex! and broadcasting rules #1646) we came to the conclusions that in such cases we should treat AbstractVector{<:AbstractVector} and NTuple{N, AbstractVector} in the same way so it is also consistent to treat them in the same way here.

The AbstractVector - Tuple duality is similar to the case of e.g. push! where we already accept both in the same way.

@pdeffebach
Copy link
Contributor

Thanks for the explanation!

Wow there are a lot of edge cases. Would it be better to throw errors due to ambiguity and then point the user towards something more specific?

@bkamins
Copy link
Member Author

bkamins commented Feb 13, 2019

All the functions for tuples are one-liners, so it is less code to implement them than to produce a proper deprecation.
I think we do not have to support a tuple for names argument (this would remove 2 methods from the list and leave only 2 new methods). I just thought it would be consistent to support tuples also there (but I am OK to allow only vectors of names or e.g. support only vector-vector and tuple-tuple constructor and disallow vector-tuple and tuple-vector combinations).

@nalimilan
Copy link
Member

(but I am OK to allow only vectors of names or e.g. support only vector-vector and tuple-tuple constructor and disallow vector-tuple and tuple-vector combinations).

I agree we should drop methods mixing vectors and tuples. It's already complex enough to cover all reasonable cases...

@bkamins
Copy link
Member Author

bkamins commented Feb 15, 2019

OK - removed

@nalimilan nalimilan merged commit 96eb818 into JuliaData:master Feb 20, 2019
@bkamins bkamins deleted the dataframe_from_tuple branch February 20, 2019 09:42
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.

3 participants