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

fix template param as ndim is internally int #17406

Merged
merged 2 commits into from
Jan 24, 2020
Merged

fix template param as ndim is internally int #17406

merged 2 commits into from
Jan 24, 2020

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Jan 22, 2020 •
edited
Loading

Description

Remove the template param for dimtype since internally MXNet uses int (no need for uint32 in that case)
Introduced in #15593

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Code is well-documented:
  • 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

modified: src/c_api/c_api.cc

@ChaiBapchya
Copy link
Contributor Author

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

@lanking520 lanking520 added the pr-awaiting-review PR is waiting for code review label Jan 22, 2020
@ChaiBapchya
Copy link
Contributor Author

@apeforest @access2rohit plz review. Thanks

apeforest
apeforest approved these changes Jan 23, 2020
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

access2rohit
access2rohit approved these changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@access2rohit access2rohit 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.

LGTM! Can you paste your output here for reference. It's fine either way. Non-blocking req.

@access2rohit
Copy link
Contributor

@mxnet-label-bot add [pr-awaiting-merge]

@lanking520 lanking520 added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Jan 24, 2020
@apeforest apeforest merged commit 593d5b6 into apache:master Jan 24, 2020
@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Jan 24, 2020 •
edited
Loading

Verified this change works by

  1. Checking if the API MXNDArrayCreateEx64 is called
    How? By adding a LOG(INFO) in here

https://github.com/apache/incubator-mxnet/blob/4a13eddc0d388d9ee41010ac40af5c8325335619/src/c_api/c_api.cc#L930

  1. Checking if the MXNDArrayCreateEx64 API called, returns correct results
    How? By verifying test_basic
python3 -m nose tests/nightly/test_large_array.py:test_basic --verbose

@ChaiBapchya ChaiBapchya deleted the fix_create_ndarray_template branch January 24, 2020 08:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apeforest apeforest apeforest approved these changes

@access2rohit access2rohit access2rohit approved these changes

Assignees
No one assigned
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChaiBapchya @access2rohit @apeforest @lanking520