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

[Numpy] Basic indexing in symbolic interface of DeepNumpy #16621

Merged
merged 12 commits into from
Nov 27, 2019

Conversation

sxjscience
Copy link
Member

@sxjscience sxjscience commented Oct 25, 2019

Description

Continue #15905.

  1. Add support of basic symbolic indexing, currently we support the following indexing types

    • integer types
    • python slice
    • tuple of the above two types for multi-axis slicing
  2. Fixes the MKLDNN integration of MXNet, which does not support 0-size NDArray. Currently, the solution is to disable MKLDNN if the size of input is 0.

  3. Support loading and saving symbols in the Numpy interface

  4. Add two testing utility functions:

    • check_hybridize_consistency, this is added to test_utils
    • check_gluon_save_load

Once merged, this will fix #16279 and #16887

Followup work will be to support Ellipsis in the symbolic interface.

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

Changes

  • Numpy Symbolic Basic Indexing, tests
  • MKLDNN integration fix
  • Save and load numpy symbols
  • Testing utilities for checking the consistency of hybridblock

update

Update _symbol.py

fix

Update _symbol.py

fix

Update symbol.py

Update symbol.py

update

update

try to fix

fix bug

Fix for hsplit and split

fix

try to fix

try to fix

try to fix

try to fix

fix bug

fix lint

fix slicing

fix concatenate

try to not use output_is_list

Update test_numpy_gluon.py

try to fix the basic indexing

try to fix

fix

fix

debug

fix bug

debug

debug

debug

try to fix

fix

skip the zero-size ndarrays in MKLDNN

Update multiarray.py

fix bug

Update ndarray.py

remove debug

fix bug

try to fix

Update test_numpy_gluon.py

fix

add back

Update test_numpy_gluon.py

Update test_numpy_gluon.py

fix bug

try to fix

fix test
@sxjscience sxjscience changed the title [Numpy][WIP] Basic indexing in symbolic interface [Numpy] Basic indexing in symbolic interface Nov 24, 2019
@sxjscience sxjscience changed the title [Numpy] Basic indexing in symbolic interface [Numpy] Basic indexing in symbolic interface of DeepNumpy Nov 24, 2019
Test for case2

fix lint

fix

accerate testing

move check_gluon_hybridize_consistency to test_utils

update docstring

Use Size in the implementation

fix bug
@ptrendx
Copy link
Member

ptrendx commented Nov 25, 2019

@sxjscience Issue #16887 is tagged as 1.6, but this PR introduces new functionality (so probably we do not want it in 1.6). Are you going to make another PR for 1.6 branch with the targeted fix for #16887 or should we drop 1.6 tag from that issue?

@sxjscience
Copy link
Member Author

@ptrendx Is it possible to include this into 1.6? It's more like a bug-fix because in the previous version, we haven't checked the consistency of hybridization.

@sxjscience
Copy link
Member Author

@ptrendx After a second thought, I think it would be better to fix the bug first. Let me extract part of this PR to a bugfix.

The name of this symbol, returns ``None`` for list symbol.
"""
if self.num_outputs > 1:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

raise instead of return?

@@ -3125,6 +3128,26 @@ def _get_dim_size(start, stop, step):
return dim_size


def _get_slice_len_for(slc, seq_length):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: change _get_slice_len_for to _get_slice_len.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me fix it in the other PR (#16902)

python/mxnet/ndarray/numpy/_op.py Outdated Show resolved Hide resolved
python/mxnet/symbol/numpy/_symbol.py Outdated Show resolved Hide resolved
python/mxnet/symbol/numpy/_symbol.py Outdated Show resolved Hide resolved
--------
_Symbol.tojson : Used to save symbol into json string.
"""
if not isinstance(json_str, string_types):
Copy link
Contributor

Choose a reason for hiding this comment

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

Reuse mx.sym.load_json?

--------
Symbol.save : Used to save symbol into file.
"""
if not isinstance(fname, string_types):
Copy link
Contributor

Choose a reason for hiding this comment

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

Reuse mx.sym.load?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've copied the code here because we need to return the _Symbol object.

@sxjscience sxjscience merged commit a98cefc into apache:master Nov 27, 2019
@sxjscience sxjscience deleted the mikemwx-local branch December 3, 2019 01:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Inconsistency between HybridBlock and Block
3 participants