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

fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 #14107

Closed
wants to merge 3 commits into from
Closed

fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 #14107

wants to merge 3 commits into from

Conversation

mseth10
Copy link
Contributor

@mseth10 mseth10 commented Feb 9, 2019 •
edited
Loading

Description

This PR fixes nd.slice for erroneous output in the following cases:
(1) begin=end ( fixes #13760 ). Throws invalid input error.
(2) end=-1, step=-1 ( fixes #14105 ). Added tests for the same.

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 http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Fixed nd.slice for erroneous outputs

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@mseth10 mseth10 mentioned this pull request Feb 9, 2019
Closed
5 tasks
@vandanavk
Copy link
Contributor

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

@marcoabreu marcoabreu added Operator pr-awaiting-review PR is waiting for code review labels Feb 10, 2019
@mseth10 mseth10 force-pushed the fix-issue-14105 branch 3 times, most recently from dd28023 to 77d09e4 Compare February 10, 2019 09:15
szha
szha suggested changes Feb 10, 2019
View reviewed changes
src/operator/tensor/matrix_op-inl.h Outdated Show resolved Hide resolved
lupesko
lupesko suggested changes Feb 10, 2019
View reviewed changes
Copy link
Contributor

@lupesko lupesko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, but I left a comment to address:
Beyond other comments, I suggest you expand the test coverage to include the fix for #14105

tests/python/unittest/test_operator.py Outdated Show resolved Hide resolved
@roywei
Copy link
Member

roywei commented Feb 21, 2019

@mseth10 Thanks for the contribution, could you address the comments?

@mseth10 mseth10 changed the title fix nd.slice for following outputs: (1) begin=end and (2) end=-1, step=-1 [WIP] fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 Feb 21, 2019
@mseth10 mseth10 closed this Feb 22, 2019
@mseth10 mseth10 reopened this Feb 22, 2019
@mseth10 mseth10 mentioned this pull request Feb 22, 2019
Closed
7 tasks
@mseth10 mseth10 changed the title [WIP] fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 Feb 22, 2019
@mseth10
Copy link
Contributor Author

mseth10 commented Feb 22, 2019

@szha @marcoabreu I have made the requested changes. Can you please review again. Thanks!

@szha
Copy link
Member

szha commented Feb 22, 2019

explicitly expressing empty tensor is currently not possible, and it will cause many problems because of the various sanity check that assumes not legal empty tensor.

@reminisce
Copy link
Contributor

For the first case it should throw exception (LOG(FATAL)) for now as MXNet does not support zero size arrays in computation. For the second one, throwing exception is expected for the same reason.

We are planning to support zero-size arrays. But until then, the current behavior should be kept.

@mseth10 mseth10 changed the title fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 [WIP] fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 Mar 4, 2019
@mseth10
Copy link
Contributor Author

mseth10 commented Mar 4, 2019 •
edited
Loading

@mxnet-label-bot add [pr-work-in-progress]

@marcoabreu marcoabreu added the pr-work-in-progress PR is still work in progress label Mar 4, 2019
@mseth10 mseth10 closed this Mar 12, 2019
@mseth10 mseth10 deleted the fix-issue-14105 branch March 12, 2019 18:40
@mseth10
Copy link
Contributor Author

mseth10 commented Mar 12, 2019

Created new PR #14403 which does not deal with empty tensors.

@mseth10 mseth10 changed the title [WIP] fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 fix nd.slice for following inputs: (1) begin=end and (2) end=-1, step=-1 Mar 12, 2019
@mseth10 mseth10 mentioned this pull request Mar 12, 2019
Merged
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@szha szha szha requested changes

@lupesko lupesko lupesko requested changes

@marcoabreu marcoabreu marcoabreu requested changes

@reminisce reminisce Awaiting requested review from reminisce

Assignees
No one assigned
Labels
Operator pr-awaiting-review PR is waiting for code review pr-work-in-progress PR is still work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nd.slice errors out when end=-1 and step=-1 nd.slice does not return empty tensor when begin=end
7 participants
@mseth10 @vandanavk @roywei @szha @reminisce @lupesko @marcoabreu