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.

Sign up for GitHub

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

Jump to bottom

convert for DataFrameRow #1575

Merged
merged 6 commits into from
Nov 1, 2018
Merged

convert for DataFrameRow #1575

merged 6 commits into from
Nov 1, 2018

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Oct 26, 2018

PR following discussion in #1564.

nalimilan
nalimilan reviewed Oct 27, 2018
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks!

src/dataframerow/dataframerow.jl Outdated
[dfr[i] for i in 1:length(dfr)]
Base.convert(::Type{Vector{T}}, dfr::DataFrameRow) where T =
T[dfr[i] for i in 1:length(dfr)]
Base.Vector(dfr::DataFrameRow) = Base.convert(Vector, df)
Copy link
Member

Choose a reason for hiding this comment

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

No need for Base..

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting - I did not think earlier abut such case. Where is this documented?

Copy link
Member

@nalimilan nalimilan Oct 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Well, you never need to qualify calls to exported functions from other modules. You don't write Base.length either, do you? :-)

EDIT: to be clear, I was talking about the RHS.

Copy link
Member

Choose a reason for hiding this comment

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

Also, the CI failure is due to df instead of dfr.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - I thought you wanted to strip Base. from Base.Vector and got confused.

test/dataframerow.jl Outdated
@test Vector(dfr) == Real[1, 2.0]
@test eltype(Vector(dfr)) == Real
@test Vector{Int}(dfr) == [1, 2]
@test eltype(Vector{Int}(dfr)) == Int
Copy link
Member

Choose a reason for hiding this comment

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

Also test convert directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

added

@nalimilan
Copy link
Member

Actually, I'm a bit hesitant about whether convert should return a Vector or a Matrix. Vector sounds more logical if we consider DataFrameRow as equivalent to a (named) tuple, but tuples don't support conversion to vectors. Matrix is more consistent if we consider that vcat(convert(Array, DataFrameRow(df, 1)), convert(Array, DataFrameRow(df, 2))) should be equivalent to convert(Array, df[1:2,:]).

@bkamins
Copy link
Member Author

bkamins commented Oct 27, 2018

OK - we can leave it as is.

@bkamins
Copy link
Member Author

bkamins commented Oct 27, 2018

Thanks for the fixes.

@bkamins bkamins merged commit ccae564 into JuliaData:master Nov 1, 2018
@bkamins bkamins deleted the dataframerow_convert branch November 1, 2018 12:33
@nalimilan nalimilan mentioned this pull request Mar 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nalimilan nalimilan nalimilan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bkamins @nalimilan