10000 Implement array vs array functionality to `chainerx.minimum` by aksub99 · Pull Request #6541 · chainer/chainer · GitHub
[go: up one dir, main page]
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

Implement array vs array functionality to chainerx.minimum #6541

Merged
merged 20 commits into from
Apr 5, 2019
Merged

Implement array vs array functionality to chainerx.minimum #6541

merged 20 commits into from
Apr 5, 2019

Conversation

aksub99
Copy link
Contributor
@aksub99 aksub99 commented Mar 15, 2019

Implemented array vs array functionality to chainerx.minimum
Ref. #6423

@hvy hvy added cat:feature Implementation that introduces new interfaces. ChainerX Related to ChainerX. labels Mar 18, 2019
@aksub99
Copy link
Contributor Author
aksub99 commented Mar 18, 2019

@asi1024 Could you please let me know if the Travis failures are related to this PR?

@asi1024
Copy link
Member
asi1024 commented Mar 19, 2019

Thanks for the PR! Could you fix for ClangFormat like the following link?
https://travis-ci.org/chainer/chainer/jobs/506722734#L753

@aksub99
Copy link
Contributor Author
aksub99 commented Mar 19, 2019

@asi1024 Made the required changes.

@asi1024
Copy link
Member
asi1024 commented Mar 19, 2019

Could you add a test in tests/chainerx_tests/unit_tests/routines_tests/test_math.py?

@aksub99
Copy link
Contributor Author
aksub99 commented Mar 19, 2019

Unintentionally committed some mistakes in the python test for chainerx.minimum . Am in the process of rectifying them.

asi1024
asi1024 requested changes Mar 20, 2019
View reviewed changes
@asi1024 asi1024 mentioned this pull request Mar 20, 2019
Merged
@aksub99 aksub99 force-pushed the Implement_array_vs_array_min branch from 2fd2f3a to b62fcc6 Compare March 21, 2019 06:40
@aksub99
Copy link
Contributor Author
aksub99 commented Mar 21, 2019 •
edited
Loading

/home/akshay/chainer/chainerx_cc/chainerx/routines/math.cc:61:7: note: template argument deduction/substitution failed: /home/akshay/chainer/chainerx_cc/chainerx/routines/math.cc:596:54: note: couldn't deduce template parameter ‘Impl’ return BroadcastQuaternary(&IfGreaterElse, x1, x2); // x1 > x2 ? x2 : x1

I get this error after making the above changes. I think this is probably because the return type of IfGreaterElse is Array and not void and hence is not directly deduced. @asi1024 Could you suggest a method of overcoming this?

@chainer-ci
Copy link
Member
chainer-ci 8000 commented Mar 21, 2019

Can one of the admins verify this patch?

@asi1024
Copy link
Member
asi1024 commented Mar 22, 2019

Hmm, how about defining MinimumImpl(const Array& x1, const Array& x2, const Array& out) and using BroadcastBinary? we may not need to define BroadcastQuaternary only for this case.

@aksub99 aksub99 force-pushed the Implement_array_vs_array_min branch from b8eab18 to 08fadda Compare March 22, 2019 07:42
@aksub99
Copy link
Contributor Author
aksub99 commented Mar 22, 2019

@asi1024 Could you please review changes?

@aksub99
Copy link
Contributor Author
aksub99 commented Mar 22, 2019

Is this fine @asi1024 ?

@aksub99 aksub99 force-pushed the Implement_array_vs_array_min branch from 977f797 to 37a147f Compare March 22, 2019 09:49
@hvy hvy mentioned this pull request Mar 26, 2019
Closed
16 tasks
@aksub99 aksub99 force-pushed the Implement_array_vs_array_min branch from 1545976 to 16e4b19 Compare March 30, 2019 16:34
@aksub99
Copy link
Contributor Author
aksub99 commented Mar 30, 2019

@asi1024 I will support mixed dtypes in the next commit.

@aksub99 aksub99 force-pushed the Implement_array_vs_array_min branch from 47bd5e0 to b2eeca9 Compare March 30, 2019 18:13
@aksub99
Copy link
Contributor Author
aksub99 commented Mar 30, 2019

@asi1024 Could you please review?

asi1024
asi1024 requested changes Mar 31, 2019
View reviewed changes
6D47
@aksub99
Copy link
Contributor Author
aksub99 commented Mar 31, 2019 •
edited
Loading

I have made all the suggested changes. But when I test my implementation locally, I get the following warning. Could you please take a look?
chainer/testing/_bundle.py:0 /home/akshay/chainer/chainer/testing/_bundle.py:0: PytestWarning: cannot collect test class "TestMinimum_param_139_{x1_shape=(2,), x2_shape=(3, 2), in_dtypes=('float64', 'float64'), out_dtype='float64'}" because it has a __init__ constructor

asi1024
asi1024 requested changes Apr 3, 2019
View reviewed changes
@aksub99
Copy link
Contributor Author
aksub99 commented Apr 4, 2019

@asi1024 Could you please review the modified tests?

@asi1024
Copy link
Member
asi1024 commented Apr 5, 2019

Jenkins, test this please.

@chainer-ci
Copy link
Member

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

@asi1024
Copy link
Member
asi1024 commented Apr 5, 2019

The test failures are unrelated to this PR. LGTM! @aksub99 Thank you very much!

@asi1024 asi1024 merged commit 6ce5c9e into chainer:master Apr 5, 2019
@asi1024 asi1024 added this to the v7.0.0a1 milestone Apr 5, 2019
@asi1024 asi1024 added the to-be-backported Pull request that should be backported. label Apr 9, 2019
asi1024 added a commit to asi1024/chainer that referenced this pull request Apr 9, 2019
@asi1024
Merge pull request chainer#6541 from aksub99/Implement_array_vs_array…
2d784df 
…_min

Implement array vs array functionality to chainerx.minimum
@asi1024 asi1024 mentioned this pull request Apr 9, 2019
Merged
@niboshi niboshi changed the title Implement array vs array functionality to chainerx.minimum Implement array vs array functionality to chainerx.minimum Apr 11, 2019
@chainer-ci
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asi1024 asi1024 asi1024 requested changes

Assignees

@asi1024 asi1024

Labels
cat:feature Implementation that introduces new interfaces. ChainerX Related to ChainerX. to-be-backported Pull request that should be backported.
Projects
None yet
Milestone
v7.0.0a1
Development

Successfully merging this pull request may close these issues.
4 participants
@aksub99 @asi1024 @chainer-ci @hvy
0