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

Fix ndarray indexing bug #16895

Merged
merged 3 commits into from Dec 5, 2019
Merged

Conversation

reminisce
Copy link
Contributor

Description

Fixed #16647

@fhieber

@reminisce reminisce requested a review from szha as a code owner November 24, 2019 07:03
@reminisce reminisce added this to In progress in numpy via automation Nov 24, 2019
src/ndarray/ndarray.cc Outdated Show resolved Hide resolved
@ptrendx
Copy link
Member

ptrendx commented Nov 25, 2019

@szha Can we get this reviewed today?

@reminisce Since the repro in #16647 is small, can we add a test with it to make sure the original issue is (and stays) actually fixed?

@reminisce
Copy link
Contributor Author

@ptrendx Thanks for the review. I added similar test case in test_numpy_ndarray.py, but I have also included the cases from #16647 in test_ndarray.py as you requested.

@reminisce
Copy link
Contributor Author

@PatricZhao @TaoLv MKLDNN CPP test keeps failing on FullyConnectedOp in this PR. Could you guys help to take a look? Thanks.
http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-gpu/detail/PR-16895/5/pipeline/366

@TaoLv
Copy link
Member

TaoLv commented Nov 26, 2019

@reminisce I have created a PR to revert the problematic commit. Sorry for the inconvenience.

@ptrendx
Copy link
Member

ptrendx commented Nov 26, 2019

@reminisce could you merge with the latest master to pick up this fix for the cpp test?

@ptrendx
Copy link
Member

ptrendx commented Nov 27, 2019

Build failed due to network error - I retriggered it.

@reminisce
Copy link
Contributor Author

Ubuntu 14.04 has problems. It's being under investigation.

numpy automation moved this from In progress to Reviewer approved Dec 5, 2019
Copy link
Contributor

@haojin2 haojin2 left a comment

Choose a reason for hiding this comment

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

LGTM

@haojin2 haojin2 merged commit 10903e8 into apache:master Dec 5, 2019
numpy automation moved this from Reviewer approved to Done Dec 5, 2019
ptrendx pushed a commit to ptrendx/mxnet that referenced this pull request Dec 9, 2019
* Fix indexing bug

* More test cases

* Add test from 16647
ptrendx added a commit that referenced this pull request Dec 10, 2019
* Fix ndarray indexing bug (#16895)

* Fix indexing bug

* More test cases

* Add test from 16647

* [Gluon] Update contrib.Estimator LoggingHandler to support logging per batch interval (#16922)

* Update LoggingHandler to support logging per interval

* Fix the constant variable issue in the logging handler

* Remove the constant variable hack in the logging handler.

* 1) replace LOG_PER_BATCH with LOG_PER_INTERVAL 2) add test case

* Improve the test script for LoggingHandler

* small fix on the test script

* logging handler test case bug fix

* remove parameter verbose from LoggingHandler

* move log_interval to the first argument

* resolve unittest mistakes

* Add micro averaging strategy to pearsonr metric (#16878)

        Strategy to be used for aggregating across mini-batches.
            "macro": average the pearsonr scores for each batch.
            "micro": compute a single pearsonr score across all batches.

* [Bugfix] [Numpy] Add `kAddTo` and kNullOp to Transpose (#16979)

* update

Check for repeated axes

enable addto to transpose

fix

fix

fix

fix

remove unused ndim

Update pseudo2DTranspose_op-inl.cuh

Update pseudo2DTranspose_op-inl.cuh

Update pseudo2DTranspose_op-inl.cuh

fix

Update pseudo2DTranspose_op-inl.cuh

try to fix

Update pseudo2DTranspose_op-inl.cuh

Update pseudo2DTranspose_op-inl.cuh

Update pseudo2DTranspose_op-inl.cuh

fix

Update np_matrix_op.cc

Update test_numpy_op.py

update test case

fix implementation

fix bug

update

fix bug

Update pseudo2DTranspose_op-inl.cuh

fix

fix

Update test_numpy_op.py

* Fix bug

* fix docstring

* try to address comment

* no need to change this line

* Fix bug

* address comments

* address comment

* introduce  gradient update handler to the  base estimator (#16900)

* introduce  gradient update handler to the  base estimator

* Modify the gradient update handler to include the batch size

* Remove unrelated gradient update handler.

* Modify gradient update handler to take the current batch size.

* Remove white space to avoid the sanity check failure

* add small tweak to the handler code

* Modify the documentation of priority parameter of relevant handlers.

* small modification on the documentation.

* Add small modification on the documentation.

* Remove unnecessary list check
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
numpy
  
Done
Development

Successfully merging this pull request may close these issues.

Set range in NDArray from Python list fails
4 participants