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.

Sign up for GitHub

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

Jump to bottom

Support ellipsis in Array::At and __getitem__ #7561

Merged
merged 2 commits into from
Jul 3, 2019
Merged

Support ellipsis in Array::At and __getitem__ #7561

merged 2 commits into from
Jul 3, 2019

Conversation

asi1024
Copy link
Member

@asi1024 asi1024 commented Jun 20, 2019 •
edited
Loading

Resolve #7560.
Depends on #7559.
Depends on #7614.

@asi1024 asi1024 added cat:enhancement Implementation that does not break interfaces. ChainerX Related to ChainerX. labels Jun 20, 2019
@asi1024 asi1024 force-pushed the chainerx-ellipsis branch from 7170572 to 231dd95 Compare June 20, 2019 04:10
@emcastillo
Copy link
Member

Just a question.
It is possible to add some explicit tests on chainerx side?

Or with just the chainer test suite should be enough

@asi1024 asi1024 mentioned this pull request Jun 20, 2019
Merged
@asi1024 asi1024 force-pushed the chainerx-ellipsis branch from 231dd95 to 45444b5 Compare June 20, 2019 06:18
@asi1024
Copy link
Member Author

asi1024 commented Jun 20, 2019

@emcastillo I added python tests for chainerx now. Usually, we don't add chainerx test in C to avoid duplications.

@asi1024 asi1024 force-pushed the chainerx-ellipsis branch from 45444b5 to bf653f0 Compare June 20, 2019 08:02
@asi1024 asi1024 changed the title [WIP] Support ellipsis in Array::At and __getitem__ [depend #7559] Support ellipsis in Array::At and __getitem__ Jun 20, 2019
@asi1024 asi1024 added ChainerX-long Long running PR for ChainerX task board and removed ChainerX-long Long running PR for ChainerX task board labels Jun 21, 2019
@asi1024 asi1024 force-pushed the chainerx-ellipsis branch 2 times, most recently from 3061501 to fb42f0a Compare June 21, 2019 08:15
@asi1024 asi1024 changed the title [depend #7559] Support ellipsis in Array::At and __getitem__ Support ellipsis in Array::At and __getitem__ Jun 21, 2019
@emcastillo
Copy link
Member

There are some errors related to indexing on travis.
Might be an issue with the configuration on the CI after upgrading pybind?

@emcastillo emcastillo self-assigned this Jun 24, 2019
@asi1024
Copy link
Member Author

asi1024 commented Jun 25, 2019 •
edited
Loading

This line expects IndexError at the end of the loop, but chainerx.ndarray currently raises DimensionError. How should we fix it?

chainer/chainer/functions/loss/negative_sampling.py

Line 299 in fb42f0a

for ik, ig, idgx_df in six.moves.zip(k, g, dgx_df):

@asi1024 asi1024 mentioned this pull request Jun 27, 2019
Closed
@asi1024 asi1024 force-pushed the chainerx-ellipsis branch from fb42f0a to e2126c2 Compare June 27, 2019 07:54
@asi1024 asi1024 changed the title Support ellipsis in Array::At and __getitem__ [depend #7614] Support ellipsis in Array::At and __getitem__ Jun 27, 2019
@niboshi niboshi added the ChainerX-long Long running PR for ChainerX task board label Jun 28, 2019
@hvy
Copy link
Member

hvy commented Jul 1, 2019

#7614 has now been merged.

@asi1024 asi1024 force-pushed the chainerx-ellipsis branch from e2126c2 to 1b1bb5a Compare July 1, 2019 05:42
@asi1024 asi1024 changed the title [depend #7614] Support ellipsis in Array::At and __getitem__ Support ellipsis in Array::At and __getitem__ Jul 1, 2019
@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 1b1bb5a, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member

Fails due to the flake8 issue a for which a PR is pending.

@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 1b1bb5a, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member

Unrelated error due to TestDeconvolutionND

@emcastillo emcastillo added this to the v7.0.0b2 milestone Jul 3, 2019
@emcastillo emcastillo removed the ChainerX-long Long running PR for ChainerX task board label Jul 3, 2019
@emcastillo emcastillo merged commit a3bc84e into chainer:master Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@emcastillo emcastillo

Labels
cat:enhancement Implementation that does not break interfaces. ChainerX Related to ChainerX.
Projects
None yet
Milestone
v7.0.0b2
Development

Successfully merging this pull request may close these issues.

Support ellipsis in Array::At and __getitem__
5 participants
@asi1024 @emcastillo @hvy @chainer-ci @niboshi