-
Notifications
You must be signed in to change notification settings - Fork 141
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
Added access_row_tuples_by_indexs method #463
Conversation
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 think, it will not work properly for dataframe having multi indexes. Can you please try with multi index and find the issue? We can have this method for dataframe having only index (not multi index).
it 'returns Array of row tuples' do | ||
expect(df.access_row_tuples_by_indexs(1)).to eq([[:b, 12]]) | ||
expect(df.access_row_tuples_by_indexs(0,3)).to eq([[:a, 52], [:d, 17]]) | ||
end |
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.
Please also add testcases for dataframe having custom indexes like index: [:one, :two, :three, :four, :five])
I tried it with multi indexes and found two different bugs which needs to be managed separately. This method will work with multi indexes only when a single multi-index is provided
|
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 think, the multi index issue must be handled in separate PR. This PR looks good. It would be better if you had added example along with above the method documentation.
Note: in dataframe.rb, it is possible to use instead:
It is about 10 times slower that the current reverted code but, among the two bugs found by @Prakriti-nith , it is not affected by the second one (ie I have:
) |
Thanks @paisible-wanderer for informing us. @Prakriti-nith , please use similar thing for dataframe having multi index and test it for different cases. |
#462
This PR aims to revert back the changes for
access_row_tuples_by_indexs
method.