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

Simplify F.embed_id test #7903

Merged
merged 8 commits into from
Aug 20, 2019
Merged

Simplify F.embed_id test #7903

merged 8 commits into from
Aug 20, 2019

Conversation

dido1998
Copy link
Contributor

@dido1998 dido1998 commented Aug 10, 2019 •
edited by emcastillo
Loading

Depends #7960
Merge after #7960

@dido1998
Copy link
Contributor Author

I am not getting this error locally, could someone have a look?

@dido1998 dido1998 mentioned this pull request Aug 12, 2019
Closed
emcastillo
emcastillo reviewed Aug 13, 2019
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

Please, reply questions.
Thanks!

@emcastillo emcastillo added cat:test Test or CI related. st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. labels Aug 13, 2019
@emcastillo emcastillo removed the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Aug 16, 2019
@emcastillo
Copy link
Member

emcastillo commented Aug 16, 2019 •
edited
Loading

#7960 should fix the testing issue

@emcastillo emcastillo added GSoC st:blocked-by-another-pr State indicating that another ticket is preventing this ticket from being closed/merged. labels Aug 16, 2019
emcastillo
emcastillo requested changes Aug 16, 2019
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

please checkout the comments,

thanks! :)

tests/chainer_tests/functions_tests/connection_tests/test_embed_id.py Outdated Show resolved Hide resolved
tests/chainer_tests/functions_tests/connection_tests/test_embed_id.py Outdated Show resolved Hide resolved
tests/chainer_tests/functions_tests/connection_tests/test_embed_id.py Outdated Show resolved Hide resolved
@emcastillo
Copy link
Member

@dido1998 can you include this code in your PR?
#7960

lets commit it all together.
Thanks!

@emcastillo emcastillo mentioned this pull request Aug 19, 2019
Closed
@emcastillo
Copy link
Member

emcastillo commented Aug 19, 2019 •
edited
Loading

please, Fix flake8

/home/travis/build/chainer/chainer/chainerx/_fallback_workarounds.py:126:1: E303 too many blank lines (3)

@dido1998
Copy link
Contributor Author

It is still not working. Are more changes required to the code added in #7960 ?

@emcastillo
Copy link
Member

what is not working?
travis checks were ok

@emcastillo
Copy link
Member

Jenkins, test this please

@dido1998
Copy link
Contributor Author

If I put ignore_label : 1 instead of None then I get this error module 'chainerx' has no attribute 'broadcast_arrays'

@emcastillo
Copy link
Member

emcastillo commented Aug 19, 2019 •
edited
Loading

does chainerx works in your env?
I tested it with the ignore_label and stand alone and broadcast_arrays worked well

@dido1998
Copy link
Contributor Author

yes

@emcastillo
Copy link
Member

emcastillo commented Aug 19, 2019 •
edited
Loading

try import chainerx
chainerx.broadcast_arrays

if it doesnt work paste here the output of print(dir(chainerx))

@dido1998
Copy link
Contributor Author

@emcastillo its working now!

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 38e20d2, target branch master) succeeded!

@emcastillo
Copy link
Member

jenkins, test this please

@emcastillo emcastillo added st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. and removed st:blocked-by-another-pr State indicating that another ticket is preventing this ticket from being closed/merged. labels Aug 19, 2019
@emcastillo emcastillo added this to the v7.0.0b3 milestone Aug 19, 2019
emcastillo
emcastillo approved these changes Aug 19, 2019
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@chainer-ci
Copy link
Member

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

@emcastillo
Copy link
Member

Jenkins, test this please

@niboshi niboshi changed the title simplify embed-id test Simplify F.embed_id test Aug 20, 2019
@chainer-ci
Copy link
Member

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

@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

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

@dido1998
Copy link
Contributor Author

I think the error is not related to this PR

@emcastillo emcastillo merged commit 2911c39 into chainer:master Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emcastillo emcastillo emcastillo approved these changes

Assignees

@emcastillo emcastillo

Labels
cat:test Test or CI related. GSoC st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Milestone
v7.0.0b3
Development

Successfully merging this pull request may close these issues.
3 participants
@dido1998 @emcastillo @chainer-ci