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

ENH/PERF SparseArray.take indexing #12796

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@sinhrks
Member

sinhrks commented Apr 4, 2016

  • related to #4400 (not close yet as SparseDataFrame indexing test is not sufficient)
  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry

Added more tests for sparse indexing. Fixed followings:

  • SparseArray.take has optimized logic to omit dense np.ndarray creation.
  • SparseSeires.iloc can work with negative indices. Made SparseArray.take to handle negative indices as the same rule as Index (#12676)

@sinhrks sinhrks added this to the 0.18.1 milestone Apr 4, 2016

@@ -307,13 +316,14 @@ def _get_val_at(self, loc):
else:
return _index.get_value_at(self, sp_loc)
def take(self, indices, axis=0):

This comment has been minimized.

@jreback

jreback Apr 4, 2016

Contributor

maybe use the shared_docs here?

@jreback

jreback Apr 4, 2016

Contributor

maybe use the shared_docs here?

This comment has been minimized.

@sinhrks

sinhrks Apr 5, 2016

Member

Done.

@sinhrks

sinhrks Apr 5, 2016

Member

Done.

indices = np.atleast_1d(np.asarray(indices, dtype=int))
# allow -1 to indicate missing values
indices = com._ensure_platform_int(indices)

This comment has been minimized.

@jreback

jreback Apr 4, 2016

Contributor

I realize that maybe could move _assert_take_fillable (and the shared docs for it), to another Mixin (so can use in Sparse). maybe.

@jreback

jreback Apr 4, 2016

Contributor

I realize that maybe could move _assert_take_fillable (and the shared docs for it), to another Mixin (so can use in Sparse). maybe.

This comment has been minimized.

@sinhrks

sinhrks Apr 5, 2016

Member

Because the impls are different, it is not likely to be merged. One idea is split boundary check to common._validate_take_boudary()?

@sinhrks

sinhrks Apr 5, 2016

Member

Because the impls are different, it is not likely to be merged. One idea is split boundary check to common._validate_take_boudary()?

This comment has been minimized.

@jreback

jreback Apr 5, 2016

Contributor

yeh, make this is a function that (for now), put with the .take functions in core/common.py (though I am going to move them I think to algorithms.py (so can put there in anticipation).

@jreback

jreback Apr 5, 2016

Contributor

yeh, make this is a function that (for now), put with the .take functions in core/common.py (though I am going to move them I think to algorithms.py (so can put there in anticipation).

This comment has been minimized.

@jreback

jreback Apr 5, 2016

Contributor

yep, see my comment

@jreback

jreback Apr 5, 2016

Contributor

yep, see my comment

This comment has been minimized.

@sinhrks

sinhrks Apr 6, 2016

Member

Ah, this can't be simple because IndexError is handled here rather than numpy. Can I leave it ATM?

@sinhrks

sinhrks Apr 6, 2016

Member

Ah, this can't be simple because IndexError is handled here rather than numpy. Can I leave it ATM?

This comment has been minimized.

@jreback

jreback Apr 6, 2016

Contributor

sure! note I just merged the reorg of common.py (though it may not affect) as its just some import changes.

@jreback

jreback Apr 6, 2016

Contributor

sure! note I just merged the reorg of common.py (though it may not affect) as its just some import changes.

@jreback

View changes

Show outdated Hide outdated pandas/sparse/array.py Outdated
@jreback

View changes

Show outdated Hide outdated pandas/sparse/array.py Outdated
@jreback

View changes

Show outdated Hide outdated pandas/sparse/array.py Outdated
@jreback

View changes

Show outdated Hide outdated pandas/src/sparse.pyx Outdated
@jreback

View changes

Show outdated Hide outdated pandas/src/sparse.pyx Outdated
@jreback

View changes

Show outdated Hide outdated pandas/src/sparse.pyx Outdated
@sinhrks

This comment has been minimized.

Show comment
Hide comment
@sinhrks

sinhrks Apr 7, 2016

Member

@jreback rebased, and now green.

Member

sinhrks commented Apr 7, 2016

@jreback rebased, and now green.

if allow_fill and fill_value is not None:
# allow -1 to indicate self.fill_value,
# self.fill_value may not be NaN
if (indices < -1).any():

This comment has been minimized.

@jreback

jreback Apr 7, 2016

Contributor

so if you can make an issue at some point we can move this stuff (and combin with the guts of assert_fillable_take as an internal function in core/algorighthms.py, but ok for now.

@jreback

jreback Apr 7, 2016

Contributor

so if you can make an issue at some point we can move this stuff (and combin with the guts of assert_fillable_take as an internal function in core/algorighthms.py, but ok for now.

@jreback jreback closed this in c90cdde Apr 7, 2016

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Apr 7, 2016

Contributor

thanks @sinhrks

Contributor

jreback commented Apr 7, 2016

thanks @sinhrks

@sinhrks sinhrks deleted the sinhrks:sparse_test_at branch Apr 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment