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

Fix (log_)softmax backward on empty ndarray #18711

Closed
wants to merge 1 commit into from

Conversation

bgawrych
Copy link
Contributor

Description

This pull request fixes #18710 issue. Fixed in the same way as forward pass

Checklist

Essentials

  • 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)
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@mxnet-bot
Copy link

Hey @bgawrych , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [unix-gpu, clang, unix-cpu, windows-gpu, centos-gpu, website, windows-cpu, miscellaneous, edge, centos-cpu, sanity]


Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@wkcn
Copy link
Member

wkcn commented Jul 15, 2020

Could you please add two unit-tests softmax and log_softmax with empty ndarray into tests/python/unittest/test_operator.py ? Check their backward procedure when input is empty.

Thank you!

@bgawrych
Copy link
Contributor Author

Could you please add two unit-tests softmax and log_softmax with empty ndarray into tests/python/unittest/test_operator.py ? Check their backward procedure when input is empty.

Thank you!

@wkcn Please correct me if I'm wrong but doesn't empty array (e.g. [3,0,4]) is allowed only when numpy semantics is turned on and legacy mode doesn't support this?

@wkcn
Copy link
Member

wkcn commented Jul 15, 2020

@bgawrych sorry for my mistake. We does not need to add test for legacy operator.
You can add test into tests/python/unittest/test_numpy_op.py

@bgawrych
Copy link
Contributor Author

@wkcn It's already done :) Function test_npx_softmax (tests/python/unittest/test_numpy_op.py:1916) is checking empty forward and backward pass

@bgawrych
Copy link
Contributor Author

@mxnet-bot run ci [centos-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [centos-cpu]

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@wkcn
Copy link
Member

wkcn commented Jul 15, 2020

@mxnet-bot run ci [centos-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [centos-cpu]

@DickJC123
Copy link
Contributor

DickJC123 commented Jul 16, 2020

I'm working on a PR that is mostly failing due to the issues this PR fixes, so I'll be happy to see this merged. Are the CI failures you're seeing related to this commit, thus requiring a follow-up commit, or are you done? I'm thinking about cherry-picking your present commit onto my PR so I can get to a cleaner CI.

@szha
Copy link
Member

szha commented Jul 16, 2020

The errors seem unrelated. @mxnet-bot run ci [centos-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [centos-cpu]

@DickJC123
Copy link
Contributor

DickJC123 commented Jul 16, 2020

My PR is having the same 'worker crash' problem this PR is having. Without understanding the root cause, I tried marking the failing test in unittest/test_sparse_operator.py as serial, and I got no similar failures. So you might give this a try:

@with_seed()
@pytest.mark.serial
def test_elemwise_binary_ops():

@bgawrych
Copy link
Contributor Author

Closing, as fix have been merged to master in #18694

@bgawrych bgawrych closed this Jul 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Numpy] Backward of softmax, logsoftmax failed on empty ndarray
5 participants