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

Loosen constructor for a DataFrame #1108

Merged
merged 6 commits into from
Oct 18, 2016
Merged

Conversation

andyferris
Copy link
Member

It seems to be a little more difficult than it should be to pass a container of columns to the DataFrame's constructor. This change lets you, e.g.

DataFrame([[1,2,3]], [:a])

Before this would complain because [[1,2,3]] is of type Vector{Vector{Int}} which is not a subtype of (concrete) type Vector{Any} (type parameters aren't convariant). This will automatically loosen the type parameter for the column container, if necessary.

It seems to be a little more difficult than it should be to pass a container of columns to the `DataFrame`'s constructor. This change lets you, e.g.

```julia
DataFrame([[1,2,3]], [:a])
```

Before this would complain because `[[1,2,3]]` is of type `Vector{Vector{Int}}` which is not a subtype of (concrete) type `Vector{Any}` (type parameters aren't convariant). This will automatically loosen the type parameter for the column container, if necessary.
@@ -102,9 +102,9 @@ function DataFrame(; kwargs...)
return result
end

function DataFrame(columns::Vector{Any},
function DataFrame{T}(columns::Vector{T},
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the T AFAICT. Also, while you're at it, change this to AbstractVector.

@nalimilan
Copy link
Member

Why not, though this triggers an ambiguity error that should be fixed. Could you add a test too?

@nalimilan
Copy link
Member

Thanks. Still needs a test though.

Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

Test please 🙂

@andyferris
Copy link
Member Author

OK. I'm done now. Sorry, I've only had a few seconds here and there.

@@ -185,6 +185,7 @@ module TestDataFrame
@test typeof(df[:,:a]) == NullableVector{Int}
@test typeof(df[:,:b]) == NullableVector{Char}

@test DataFrame([[1,2,3], [2.5,4.5,6.5]], [:A, :B]) = DataFrame(A = [1,2,3], B = [2.5,4.5,6.5])
Copy link
Member

Choose a reason for hiding this comment

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

This is causing syntax errors:

FAILED: dataframe.jl
LoadError: syntax: "[[1,2,3],[2.5,4.5,6.5]]" is not a valid function argument name
 in macro expansion; at C:\Users\appveyor\.julia\v0.5\DataFrames\test\runtests.jl:43 [inlined]
 in anonymous at .\<missing>:?
 in include_from_node1 at .\loading.jl:488
 in process_options at .\client.jl:262
 in _start at .\client.jl:318
while loading C:\Users\appveyor\.julia\v0.5\DataFrames\test\dataframe.jl, in expression starting on line 188

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, that is a really strange error message. The problem was I used = instead of ==

Sorry, like I said I only have a few moments here and there so I used the github web interface and relying on CI for testing.

@andyferris
Copy link
Member Author

andyferris commented Oct 17, 2016

Hmm now == for DataFrame doesn't seem to do what I expected... (it is false if the eltypes aren't the same... I was expecting the same behaviour as Array ([1,2,3] == [1.0,2.0,3.0])

EDIT: the culprit is Nullable(1) == 1 is false. Grr. Hopefully tests pass this time.

`==` between `Nullable{T}` and `T` is always false, for some reason.
@nalimilan nalimilan merged commit 10c4423 into JuliaData:master Oct 18, 2016
@nalimilan
Copy link
Member

Thanks!

nalimilan pushed a commit that referenced this pull request Jul 8, 2017
nalimilan pushed a commit that referenced this pull request Jul 8, 2017
rofinn pushed a commit that referenced this pull request Aug 17, 2017
nalimilan pushed a commit that referenced this pull request Aug 25, 2017
quinnj pushed a commit that referenced this pull request Sep 2, 2017
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