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

make rowiterator an abstractvector #50

Closed
wants to merge 3 commits into from
Closed

make rowiterator an abstractvector #50

wants to merge 3 commits into from

Conversation

piever
Copy link

@piever piever commented Dec 7, 2018

This I think simplifies the code and adds feature as we can use the AbstractVector machinery to iterate and get an element by index / slice the table etc...

@nalimilan
Copy link
Member

Makes sense, but you need to define Base.IndexStyle to return IndexLinear too I think.

@quinnj
Copy link
Member

quinnj commented Dec 11, 2018

Can we do some performance tests to just make sure this doesn't affect anything there?

@piever
Copy link
Author

piever commented Dec 11, 2018

Sure, I'll try to find some time today: I should probably specify that this abstractvector prefers linear indexing (as suggested above) and use @boundscheck and @inbounds to make sure we only do inbounds check when necessary (i.e, not when iterating the vector).

EDIT: actually the @inbounds change is maybe best left to a later PR as it may be not 100% trivial

Do you have some standard benchmarks for iteration?

@quinnj
Copy link
Member

quinnj commented Dec 12, 2018

I don't have anything standard, I usually use these lines from CSV to sanity check things (https://github.com/JuliaData/CSV.jl/blob/master/test/files.jl#L56); the pandas csv file is big enough to hit the row-iteration and the buildcolumns call hits the no-schema path.

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