Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Implemented final two binary ops, added default params for functionality #17407

Merged
merged 4 commits into from
Feb 6, 2020

Conversation

connorgoggins
Copy link
Contributor

@connorgoggins connorgoggins commented Jan 22, 2020

Description

Added implementation for reshape_like and choose_element_0index in opperf, as well as supporting variables in rules/default_params.py.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • M benchmark/opperf/nd_operations/binary_operators.py
  • M benchmark/opperf/utils/op_registry_utils.py
  • M benchmark/opperf/rules/default_params.py
  • M benchmark/opperf/opperf.py

Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your first contribution! More to come!

@ChaiBapchya
Copy link
Contributor

Please add results seen on CPU/GPU for reference.
Did you run opperf for all operators after this change?

@ChaiBapchya
Copy link
Contributor

A gist link with the results would do

@connorgoggins
Copy link
Contributor Author

@ChaiBapchya https://gist.github.com/connorgoggins/294f55595311c3b811662af08a787f2a

@ChaiBapchya
Copy link
Contributor

Also you could run the opperf utility by specifying the file format and name of the file
That is the results file I am referring to. Not the stdoutput.
Results file will spit out forward/backward time, memory consumption etc.

@access2rohit
Copy link
Contributor

access2rohit commented Jan 24, 2020

@mxnet-label-bot update [pr-awaiting-review]

@lanking520 lanking520 added pr-awaiting-merge Review and CI is complete. Ready to Merge pr-awaiting-review PR is waiting for code review and removed pr-awaiting-merge Review and CI is complete. Ready to Merge labels Jan 24, 2020
@ChaiBapchya
Copy link
Contributor

Also it would make sense to add results for

  1. Individual operator test (of the 2 added ops)
  2. Group of operator test (binary_op in this case)
  3. Full OpPerf test (as mentioned in my previous comment)

Just to ensure it is fool-proof.

@connorgoggins
Copy link
Contributor Author

connorgoggins commented Jan 25, 2020

Individual operator test (GPU): https://gist.github.com/connorgoggins/c74272c24c1e627a6db3709c1b17ee1d
Individual operator test (CPU): https://gist.github.com/connorgoggins/5e86b4a5def363eb2d791592296cd607

Group of operator test - all binary ops (GPU): https://gist.github.com/connorgoggins/948e5cb3d13ff53bfd6b4279abe28d4f
Group of operator test - all binary ops (CPU): https://gist.github.com/connorgoggins/0a5fb08543d164fce68e3c5006b0b264

Full OpPerf test (GPU)*: https://gist.github.com/connorgoggins/cceb63a3d2d17e36a7f26ed7c0c75505
Full OpPerf test (CPU): https://gist.github.com/connorgoggins/1e88cbd6764a74fc2f72b4eb5b7f022a

*Note: couldn't run sorting/searching ops tests in the full test on GPU because they exceeded the device memory of my p2.8xl instance. Will do another run on a p2.16xl to get those results, but regardless my implementation of these two binary operators has no bearing on the logic of sorting/searching ops.

@ChaiBapchya
Copy link
Contributor

Let's add ElementwiseSum op too

https://mxnet.incubator.apache.org/api/python/docs/api/ndarray/ndarray.html#mxnet.ndarray.ElementWiseSum

@connorgoggins
Copy link
Contributor Author

Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@ChaiBapchya
Copy link
Contributor

Not in this PR but in one of the other 3 PRs that you have open you can add yourself to the Contributors.md file
Welcome to the group!! @connorgoggins

@ChaiBapchya
Copy link
Contributor

ChaiBapchya commented Feb 4, 2020

Incorrect way to rebase @connorgoggins

First get your master branch updated

git remote add upstream https://github.com/apache/incubator-mxnet
git checkout master
git fetch upstream master
git merge upstream/master
git push origin master

Once your fork master is sync'ed with remote master, rebase your branch on master

git checkout <branchname>
git rebase master
git push origin <branchname>

Let me know if this works!

@connorgoggins connorgoggins force-pushed the implement_misc_binary_ops branch 2 times, most recently from 9b917a5 to 85b2f07 Compare February 5, 2020 00:19
@apeforest apeforest merged commit e8a317e into apache:master Feb 6, 2020
zheyuye pushed a commit to zheyuye/incubator-mxnet that referenced this pull request Feb 19, 2020
…ity (apache#17407)

* Implemented final two binary ops, added default params for functionality

* Removed extraneous param checks

* Added support for ElementWiseSum

* Moved ElementWiseSum to binary_element_wise_operators
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request May 29, 2020
…ity (apache#17407)

* Implemented final two binary ops, added default params for functionality

* Removed extraneous param checks

* Added support for ElementWiseSum

* Moved ElementWiseSum to binary_element_wise_operators
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants