Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Unify eachcol and columns functions #1590
Unify eachcol and columns functions #1590
Changes from 34 commits
4f9c61a
65a2d1e
c9b8e88
ebc6835
97af6cc
a470817
17b28a0
c3663bf
5a2e30c
4da925c
1cd3d3c
0fd91aa
7f10bba
717f47b
22f2b90
32f2fb2
4bd4f3c
f8a4dab
f584ffb
9bda15d
a317e77
a776b64
c882551
4536033
91edc19
f57af37
4566656
ceb697c
82aa836
52e9010
e42b8d3
b63ccba
4576df6
213106f
d9c8a30
e1cb969
912f1b4
2363ef5
6d1eb90
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@propagate_inbounds
wouldn't hurt here and below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK it will not change anything significantly as we do boundschecking anyway when we try to access column number
j
fromdf
(note thatdf
is anAbstractDataFrame
so we do not know at this point how it stores data).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but propagating this is better anyway, and the data frame implementation could use
@boundscheck
at some point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have opened a separate issue #1594 for this as this PR is complex enough and I am not sure if it will help anything (performancewise - disabling checking bounds saves us nanoseconds but then we have a dynamic dispatch on the result which is orders of magnitude more expensive).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying it matters, but it's cheap to add, so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - I will add, but I leave the issue open for further analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put this in deprecated.jl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be simpler for me to clean it up later (and I assume I will do it just after the coming release).
deprecated.jl has 62kb and almost 2000 loc now and actually it is easy to forget to fix something there.