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

Support list like label based indexing #27

Merged
merged 5 commits into from
Nov 30, 2017
Merged

Support list like label based indexing #27

merged 5 commits into from
Nov 30, 2017

Conversation

kayibal
Copy link

@kayibal kayibal commented Nov 28, 2017

Implements #25

@codecov
Copy link

codecov bot commented Nov 28, 2017

Codecov Report

Merging #27 into master will decrease coverage by 0.61%.
The diff coverage is 82.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   82.85%   82.23%   -0.62%     
==========================================
  Files           6        6              
  Lines         729      698      -31     
==========================================
- Hits          604      574      -30     
+ Misses        125      124       -1
Impacted Files Coverage Δ
sparsity/sparse_frame.py 86.18% <82.05%> (+0.57%) ⬆️
sparsity/dask/io.py 61.53% <0%> (-17.88%) ⬇️
sparsity/dask/indexing.py 100% <0%> (+13.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e61fa5f...692b58b. Read the comment docs.

Copy link

@michcio1234 michcio1234 left a comment

Choose a reason for hiding this comment

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

Two more tests, bitte, and it will be ok for me.

def test_label_based_indexing_idx(sample_frame_labels):
key = ['X', 'Y', 'Z']
results = [
sample_frame_labels.loc[key],

Choose a reason for hiding this comment

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

Just to be sure, could you please check also .loc[key, :]?

Copy link
Author

Choose a reason for hiding this comment

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

done

def test_label_based_col_and_idx(sample_frame_labels):
key = ['V', 'W'], ['A', 'B']
results = [
sample_frame_labels.loc[key],

Choose a reason for hiding this comment

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

I think indexing with .loc[(A, B)] is not necessarily the same as indexing with .loc[A, B]. Could you please check the second option? Actually, I'm surprised that the tuple works here (it works also in pd.DataFrame) because it's not mentioned here: https://pandas.pydata.org/pandas-docs/stable/generated/pandas.DataFrame.loc.html . I thought it's reserved for multiindex.

Copy link
Author

@kayibal kayibal Nov 30, 2017

Choose a reason for hiding this comment

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

Hm actually it's both the same if you use the brackets internally the object's getitem method will be called (which supports only a single argument) with the argument inside the brackets which in both cases evaluates to a tuple first.

Choose a reason for hiding this comment

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

I'd rather stick to the [A, B] syntax just to be sure that we're independent of any internal stuff and follow the documented API.

Copy link
Author

Choose a reason for hiding this comment

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

Well as said it's the same, but I agree:

>>> 1, 2 == (1, 2)
True

For the test it is ok though to leave it like this. Else I'd have to retype the key every time...

Choose a reason for hiding this comment

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

Come on, I know that 1, 2 == (1, 2), but foo(1, 2) is not the same as foo((1, 2)).
And I'm not sure what you mean by "every time" here. Anyway, it's your decision what to do here.

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm yep but .loc[] is not really a function call :P

Yeah I didn't get your second comment then exactly

Choose a reason for hiding this comment

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

Okay, nevermind.

@kayibal
Copy link
Author

kayibal commented Nov 30, 2017

@michcio1234 Added the tests you requested :)

@michcio1234
Copy link

@kayibal you didn't push them.

@kayibal kayibal merged commit 8fafddc into master Nov 30, 2017
@kayibal kayibal deleted the sparsity-25 branch November 30, 2017 13:30
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.

None yet

2 participants