You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/08/24 10:42:16 UTC
[GitHub] [incubator-mxnet] ekdnam opened a new pull request #18997: set_lower_bound(1) so that stride is not zero
ekdnam opened a new pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997
in issue: https://github.com/apache/incubator-mxnet/issues/18942, an error was occurring where strides became zero. to solve the issue, the lower bounds of stride1 and stride2 have been set to 1.
## Description ##
(Brief description on what this PR is about)
When the correlation function is called on two arrays when the stride is zero, it results in a floating point exception as a division by zero is carried out. Thus, the lower bounds of stride1 and stride2 have been set to 1.
## 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](https://issues.apache.org/jira/projects/MXNET/issues) 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 ###
- [ ] Feature1, tests, (and when applicable, API doc)
- [ ] Feature2, tests, (and when applicable, API doc)
## Comments ##
- If this change is a backward incompatible change, why must this change be made.
- Interesting edge cases to note here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-679363584
Thanks for the fix, @ekdnam! Could you add a test here and assert that proper error will happen when the parameter is out of bound? https://github.com/apache/incubator-mxnet/blob/3c4ac19daa3e645d918692b864ea19640f7e0314/tests/python/unittest/test_operator.py#L3200-L3233
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683644440
Jenkins CI successfully triggered : [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-679434549
Thanks for triggering the test! I will add the required unit tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683539597
@mxnet-bot run ci [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-684036351
@ekdnam thanks for the fix!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam removed a comment on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam removed a comment on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-680539756
What should be the value of the strides when they will go out of bounds?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683539515
@szha thanks for making the changes! :))
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-679264637
What changes can I make to the source code so that the `unix-cpu` job succeeds?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-679052386
Hey @ekdnam , 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**: [windows-gpu, centos-gpu, unix-gpu, sanity, unix-cpu, clang, windows-cpu, edge, miscellaneous, website, centos-cpu]
***
_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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-682985967
@szha can you give someone guidelines on the tests? I am sorta new to testing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683644390
@mxnet-bot run ci [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-680539756
What should be the value of the strides when they will go out of bounds?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha edited a comment on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
szha edited a comment on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683217771
@ekdnam yes happy to help. As documented [here](https://mxnet.apache.org/community/code_guide), mxnet uses pytest as the python testing tool for development. Here's pytest's guide on [how to write tests for expected exception](https://docs.pytest.org/en/stable/assert.html#assertions-about-expected-exceptions). With that, you can write the statements to check whether an exception (and in this case the expected error should be `MXNetError`) is raised when input argument doesn't meet the lower bound.
To test this out locally, you need to:
- follow the [build from source guide](https://mxnet.apache.org/get_started/build_from_source) (we use cmake for this, configs are in [/config](https://github.com/apache/incubator-mxnet/tree/master/config), choose the one that's right for your platform) and build the change locally.
- cd into `/python` folder, and you can use `python setup.py develop` to link your local copy to your python site packages, which will enable your python to find the mxnet package you built.
- if you haven't yet, install pytest. you may also choose to install the development and testing dependencies that are exactly the same as mxnet CI. you can achieve this with
```
python -m pip install -r https://raw.githubusercontent.com/apache/incubator-mxnet/master/ci/docker/install/requirement
```
- finally, run the test and make sure it passes. make changes to the code base as needed. the complete test suite of mxnet is quite large so instead you may choose to run a specific test. see pytest's guide on [selecting the test to run](https://docs.pytest.org/en/stable/usage.html#specifying-tests-selecting-tests)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha merged pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
szha merged pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha edited a comment on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
szha edited a comment on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-679269894
@ekdnam I think the test failed due to a network issue that's unrelated to the change. You can use the command as described by the mxnet ci bot in this comment: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-679052386. for now I triggered the retry for you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683241942
Thanks a lot for such an in-depth answer!!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683425962
@szha I am currently having some issues while building mxnet from source. It may take some more time to resolve them. So I can't test myself the changes made. Hope the changes made to `test_operator.py` are enough. Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ekdnam commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
ekdnam commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-684435628
@szha sure! Thank you for the help!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683217771
@ekdnam yes happy to help. As documented [here](https://mxnet.apache.org/community/code_guide), mxnet uses pytest as the python testing tool for development. Here's pytest's guide on [how to write tests for expected exception](https://docs.pytest.org/en/stable/assert.html#assertions-about-expected-exceptions). With that, you can write the statements to check whether an exception (and in this case the expected error should be `MXNetError`) is raised when input argument doesn't meet the lower bound.
To test this out locally, you need to:
- follow the [build from source guide](https://mxnet.apache.org/get_started/build_from_source) (we use cmake for this, configs are in [/config](https://github.com/apache/incubator-mxnet/tree/master/config), choose the one that's right for your platform) and build the change locally.
- cd into `/python` folder, and you can use `python setup.py develop` to link your local copy to your python site packages, which will enable your python to find the mxnet package you built.
- if you haven't yet, install pytest. you may also choose to install the development and testing dependencies that are exactly the same as mxnet CI. you can achieve this with
```
python -m pip install -r https://raw.githubusercontent.com/apache/incubator-mxnet/master/ci/docker/install/requirement
```
- finally, run the test and make sure it passes. the complete test suite of mxnet is quite large so instead you may choose to run a specific test. see pytest's guide on [selecting the test to run](https://docs.pytest.org/en/stable/usage.html#specifying-tests-selecting-tests)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-683539614
Jenkins CI successfully triggered : [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on pull request #18997: set_lower_bound(1) so that stride is not zero
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18997:
URL: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-679269894
@ekdnam I think the test failed due to a network issue that's unrelated to the change. You can use the command as described by the mxnet ci bot in this comment: https://github.com/apache/incubator-mxnet/pull/18997#issuecomment-679052386
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org