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

Enhancement of split op with indices option #13564

Closed
wants to merge 1 commit into from
Closed

Enhancement of split op with indices option #13564

wants to merge 1 commit into from

Conversation

HyperZealot
Copy link
Contributor

Description

Support split by arbitrary ranges through addition of indices option.

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

  • Indices option for split op
  • Unit tests

Comments

larroy
larroy reviewed Dec 6, 2018
View reviewed changes
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

Are we adding the documentation about indices?

src/operator/slice_channel-inl.h Show resolved Hide resolved
src/operator/slice_channel-inl.h
size_t idx = i / trailing_size % axis_size;
size_t target = 0;
for (size_t section = 0; section < num_sections; target = section++) {
if (indices[section] > idx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do preincrement (++section) as standard C++ idiom and assign target somewhere less ofuscated?

src/operator/slice_channel-inl.h Show resolved Hide resolved
src/operator/slice_channel-inl.h Outdated
}
}
DType* target_data = out_data[target];
size_t mid_idx = idx - indices[target];
Copy link
Contributor

Choose a reason for hiding this comment

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

they can be const

src/operator/slice_channel-inl.h Outdated
}
}
DType* src_grad = out_grad[src];
size_t mid_idx = idx - indices[src];
Copy link
Contributor

Choose a reason for hiding this comment

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

can be const

src/operator/slice_channel-inl.h
}

private:
SliceChannelParam param_;
int size_;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this and num_outputs be size_t?

@HyperZealot
Copy link
Contributor Author

@larroy Docs for indices already added: /~https://github.com/apache/incubator-mxnet/pull/13564/files#diff-45b9e096a484a102b0f797a2f8817361R56

@nswamy nswamy added the pr-awaiting-review PR is waiting for code review label Dec 7, 2018
larroy
larroy suggested changes Dec 7, 2018
View reviewed changes
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution HyperZealot. I added some minor comments to make the code more maintainable.

@larroy
Copy link
Contributor

larroy commented Dec 7, 2018 •
edited
Loading

I don't see the docs that you linked. Could you please relink?

@HyperZealot
Copy link
Contributor Author

@larroy the string in the describe call on the next line is the doc for the argument (after you click into the provided link), and there's also an extra example added in the .cc file for this operator. Have you read the tutorial on how to add an operator? I feel like you're asking for more documentations but you haven't read the existing ones, please checkout Parameter Registration&Operator Registration section of this doc: https://mxnet.incubator.apache.org/faq/add_op_in_backend.html, thanks

@larroy
Copy link
Contributor

larroy commented Dec 7, 2018 •
edited
Loading

Thanks, to be specific about my question, you added an "indices" parameter but you didn't add that to the example code which you modified at the bottom of your patch. Would it make sense to use this added parameter in the examples?

@HyperZealot
Copy link
Contributor Author

@larroy There was a typo in the example, the indices argument was named sections before, so indeed there was a related example added.

@HyperZealot HyperZealot force-pushed the split_enhance branch 2 times, most recently from e4c9a61 to 4e0719b Compare December 10, 2018 19:46
@HyperZealot HyperZealot requested a review from nswamy as a code owner December 10, 2018 19:46
@HyperZealot HyperZealot mentioned this pull request Dec 20, 2018
Merged
7 tasks
@Roshrini
Copy link
Member

@HyperZealot Thanks for the contribution. Can you look into failing builds so that we can move forward with this PR?

@Roshrini
Copy link
Member

Roshrini commented Jan 2, 2019

@HyperZealot Update [pr-awaiting-response]

@anirudhacharya
Copy link
Member

@HyperZealot can you please address the comments.

@HyperZealot HyperZealot deleted the split_enhance branch January 23, 2019 21:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@larroy larroy larroy requested changes

@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290

@nswamy nswamy Awaiting requested review from nswamy nswamy is a code owner

Assignees
No one assigned
Labels
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.
5 participants
@HyperZealot @larroy @Roshrini @anirudhacharya @nswamy