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

[MXNET-978] Higher Order Gradient Support broadcast_to, broadcast_power, power, elemwise_mul and elemwise_sub and add unit test function check check_nth_order_binary #17754

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

[MXNET-978] Higher Order Gradient Support broadcast_to, broadcast_power, power, elemwise_mul and elemwise_sub and add unit test function check check_nth_order_binary #17754

wants to merge 19 commits into from

Conversation

tobecontinued
Copy link
Contributor

@tobecontinued tobecontinued commented Mar 4, 2020 •
edited
Loading

Description

  • add unittest funtinon check check_nth_order_binary
  • support higher order gradient for power, elemwise_mul and elemwise_sub.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • 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

  • add unittest funtinon check check_nth_order_binary
  • support higher order gradient for broadcast_to, broadcast_power, power, elemwise_mul and elemwise_sub.
  • unittest of higher order gradient of them.

@tobecontinued
Copy link
Contributor Author

@mxnet-bot run ci [windows-gpu, windows-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [windows-gpu, windows-cpu]

@tobecontinued
Copy link
Contributor Author

@mxnet-label-bot add [Autograd, Backend, pr-awaiting-review]

@lanking520 lanking520 added Autograd Backend Issues related to the backend of MXNet pr-awaiting-review PR is waiting for code review labels Mar 28, 2020
@tobecontinued
Copy link
Contributor Author

@apeforest @kshitij12345 could you please please review the pr. thx.

@tobecontinued tobecontinued changed the title [MXNET-978] Higher Order Gradient Support power, elemwise_mul and elemwise_sub and add unit test funcino check check_nth_order_binary [MXNET-978] Higher Order Gradient Support broadcast_to, broadcast_power, power, elemwise_mul and elemwise_sub and add unit test funcino check check_nth_order_binary Mar 30, 2020
@tobecontinued tobecontinued changed the title [MXNET-978] Higher Order Gradient Support broadcast_to, broadcast_power, power, elemwise_mul and elemwise_sub and add unit test funcino check check_nth_order_binary [MXNET-978] Higher Order Gradient Support broadcast_to, broadcast_power, power, elemwise_mul and elemwise_sub and add unit test function check check_nth_order_binary Mar 31, 2020
@tobecontinued
Copy link
Contributor Author

@mxnet-bot run ci [windows-gpu, windows-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [windows-gpu, windows-cpu]

@tobecontinued
Copy link
Contributor Author

@mxnet-bot run ci [windows-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [windows-cpu]

@tobecontinued
Copy link
Contributor Author

@anirudh2290 Could someone review the pr? thx.

@kshitij12345
Copy link
Contributor

@tobecontinued Thanks for the PR.

Apologies for the delayed response.
I'll try to go through the changes in the coming week.
Thank you again.

@tobecontinued
Copy link
Contributor Author

@kshitij12345 any update? thx.

@kshitij12345
Copy link
Contributor

@tobecontinued Apologies. Will surely do that before upcoming Monday. Thank You.

kshitij12345
kshitij12345 reviewed May 3, 2020
View reviewed changes
Copy link
Contributor

@kshitij12345 kshitij12345 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 the great work.

tests/python/unittest/test_higher_order_grad.py Outdated
# of the function, and in the gradient function, they manually defined derivative_(i-1)
# and use it to computed derivative_(1) * head_grad_(1).
# It maybe a wrong approach, because the gradient of the first gradient should compute the grad of
# derivative_(1) * head_grad_(1) instead of gradient of derivative_(1).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is taken care of. (Though I'll think some more to wrap my head more around it).

Like in the example below, grad_grad_mid considers the fact that head_grad was used in computing the first gradient,
https://github.com/apache/incubator-mxnet/blob/5fd90249cafd14910633d25d3bc370a522736ac7/src/operator/tensor/elemwise_unary_op_trig.cc#L94-L121

cc @apeforest @larroy @sxjscience

Copy link
Contributor Author

@tobecontinued tobecontinued May 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I rechecked the code. you are right, The check_nth_order_unary and high order grad of unary ops are correct. because

https://github.com/apache/incubator-mxnet/blob/8c76631caa5f349aef5d515ec95a64d2954dbcef/tests/python/unittest/test_higher_order_grad.py#L564-L566

We just used differrnt ways to handle head_gread in check_nth_order_unary and check_nth_order_binary
Thx!.

tests/cpp/operator/runner/core_op_runner_test.cc Outdated
// It will get wrong dependency for the second backward in CoreOpExecutor::GetBackwardDependency
// due to "return igrad_entries[0].node;" // and failed to call CoreOpExecutor::bwd_inputs()
// and CoreOpExecutor::bwd_outpuss() due to "CHECK_EQ(backward_.size(), 1U)";.
// { "elemwise_mul", "_backward_mul" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much about this, but I don't think removing an existing test would be a good idea.

@apeforest @larroy @sxjscience Would be better people to take the call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For more explanation, becaue the failure of the test cases are caused by defects of CoreOpExecutor, I just want a pr to achieve only one task. Maybe we need a other pr or jira to fix the issuee of CoreOpExecutor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. That makes sense.
However, I feel the mentioned people are in much better position to make the call.

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

@kshitij12345 kshitij12345 kshitij12345 left review comments

@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290

Assignees
No one assigned
Labels
Autograd Backend Issues related to the backend of MXNet pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobecontinued @mxnet-bot @kshitij12345 @lanking520