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

Fix indexing behavior in NumpyTupleDataset #200

Merged
merged 3 commits into from
Jun 27, 2018

Conversation

mottodora
Copy link
Member

@mottodora mottodora commented Jun 25, 2018

This PR contains non compatibility change.

@mottodora mottodora mentioned this pull request Jun 25, 2018
3 tasks
@codecov-io
Copy link

codecov-io commented Jun 25, 2018

Codecov Report

Merging #200 into master will increase coverage by 0.71%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   77.08%   77.79%   +0.71%     
==========================================
  Files          89       91       +2     
  Lines        3875     4067     +192     
==========================================
+ Hits         2987     3164     +177     
- Misses        888      903      +15

@@ -35,7 +35,8 @@ def __init__(self, *datasets):

def __getitem__(self, index):
batches = [dataset[index] for dataset in self._datasets]
if isinstance(index, slice):
if isinstance(index, slice) or isinstance(index, list) or\
Copy link
Member

Choose a reason for hiding this comment

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

you may write
if isinstance(index, (slice, list, numpy.ndarray)):

@@ -47,6 +55,38 @@ def test_get_item_slice_index(self, data, index):
for a, e in six.moves.zip(tuple_a, tuple_e):
numpy.testing.assert_array_equal(a, e)

@pytest.mark.parametrize('index', [numpy.asarray([2, 0])])
Copy link
Member

Choose a reason for hiding this comment

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

Can you add array with length 1?

for a, e in six.moves.zip(tuple_a, tuple_e):
numpy.testing.assert_array_equal(a, e)

@pytest.mark.parametrize('index', [[2, 0]])
Copy link
Member

Choose a reason for hiding this comment

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

Can you add list with length 1?

@mottodora
Copy link
Member Author

Fix.

Copy link
Member

@corochann corochann left a comment

Choose a reason for hiding this comment

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

LGTM

@corochann corochann merged commit e5077f5 into chainer:master Jun 27, 2018
@mottodora mottodora deleted the fix-indexing branch June 27, 2018 07:23
@mottodora mottodora mentioned this pull request Jun 29, 2018
@mottodora mottodora added this to the 0.4.0 milestone Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants