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

Added access_row_tuples_by_indexs method #463

Merged
merged 4 commits into from
Jul 7, 2018

Conversation

Prakriti-nith
Copy link
Contributor

#462
This PR aims to revert back the changes for access_row_tuples_by_indexs method.

Copy link
Member

@Shekharrajak Shekharrajak left a 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
Copy link
Member

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])

@Prakriti-nith
Copy link
Contributor Author

Prakriti-nith commented Jul 4, 2018

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

  1. I think there is a bug in #pos method of Daru::MultiIndex when multiple multi-indexes are provided. access_row_tuples_by_index will work in this case when the issue with pos method will be resolved.
    daru1
    daru2

  2. #pos method is working fine in the below case but #access_row_tuples_by_indexs gives error as it uses index for creating new rows but the provided index is not actually the valid index. It is used to get the positions.
    daru3

Copy link
Member

@Shekharrajak Shekharrajak left a 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.

@Shekharrajak
Copy link
Member

Ping @zverok , @v0dro Please look into the PR as soon as possible.

Thanks!

@paisible-wanderer
Copy link
Contributor

paisible-wanderer commented Jul 5, 2018

Note: in dataframe.rb, it is possible to use instead:

def access_row_tuples_by_indexs *indexes
  get_sub_dataframe(indexes, by_position: false).map_rows(&:to_a)
end

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:

df.access_row_tuples_by_indexs [:a, :one]
=> [[11, 1], [12, 2]]

)

@Shekharrajak
Copy link
Member

Thanks @paisible-wanderer for informing us. @Prakriti-nith , please use similar thing for dataframe having multi index and test it for different cases.

@zverok zverok merged commit e64a1d5 into SciRuby:master Jul 7, 2018
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.

4 participants