-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
MAINT: Optimize loadtxt usecols. #19618
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
Conversation
numpy/lib/npyio.py
Outdated
if usecols: | ||
vals = [vals[j] for j in usecols] | ||
if len(vals) != ncols: | ||
vals = usecols_getter(vals) | ||
elif len(vals) != ncols: |
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.
Can you explain this change to an elif
?
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.
If usecols is not None, then we already know that usecols_getter will (by construction) return a list with the right number of elements. The performance gain is negligible, but it seems just good to skip the unneeded check.
@anntzer Needs rebase. |
50b3b43
to
1d32c6e
Compare
rebased |
1d32c6e
to
45f9118
Compare
45f9118
to
fcf5bba
Compare
#19693 is in, everything needs a rebase :) |
fcf5bba
to
a59c317
Compare
7-10% speedup in usecols benchmarks; it appears that even in the single-usecol case, avoiding the iteration over `usecols` more than compensates the cost of the extra function call to usecols_getter.
a59c317
to
1767e60
Compare
Thanks @anntzer . |
7-10% speedup in usecols benchmarks; it appears that even in the
single-usecol case, avoiding the iteration over
usecols
more thancompensates the cost of the extra function call to usecols_getter.