This document is for review purpose only, It is Work in Progress and yet to discussed and agreed on dev@


Apache MXNet PR Best Practices

For contributors,
Describing your patch with enough details will help reviewers to provide you with quick feedback and there by the patch getting into the Apache MXNet repository. 

If this is a new Feature, reviewers will expect you to have discussed this on dev@ and for medium/large features have a corresponding design proposal on cwiki under Proposals.

The proposal is expected to be reviewed by at least by {_} committers of the Apache MXNet project or have passed {_} hours( Saturday+Sunday together considered 24 hours) after the proposal and a lazy consensus is assumed.
Design challenges/approaches are still a fair game during code reviews.

Target Persona: The PR template aims to target a user/developer who uses Apache MXNet 6 months later has hit a bug or an issue in their production system that this PR has made, they will dig through the commits on the file and backtrace to the PR to understand what the PR intended to do. We want to provide this person enough details to understand what the change is about so it makes it easy to follow the code and commits and make their debugging easy.


Here are a few guidelines with an example on how to structure your PR.

Describe in detail what this patch is about:
[MXNet-Scala] Adding new Parameters (dataDescriptor, labelDescriptor) to DataBatch, DataIter constructors.
Consider providing an overview here

Why is this patch required?
This change is necessary to support different data and label layouts. For example: RNN based networks require different layouts(“TNC”/”NTC”). Current implementation of DataBatch, DataIter does not support different layouts and assumes a fixed layout based on the shape of the NDArray returned in the next() call to the iterator.

How have you tested this patch?
Unit Tests — Added new Unit Tests for the new constructors/Updated DataBatch Tests. — Current Unit Tests pass.
Integration Tests - pass

Paste the results of your test run.

If it is a change to the API, does this maintain backward compatibility?
If No — describe why ?

Additionally,
If you added new/modified APIs, please add a few Examples to demonstrate the usage of the API

  • Have you added examples to show the usage of the new/modified APIs ?
    • If No Why ?
    • Add Tests to examples to make sure it continues to work.
  • Have you documented the new APIs ?
    • If No — Do you need help with this ?
  • Make sure your commit history is clean i.e, you provide good description to your commits.
  • No labels

1 Comment

  1. Naveen - thanks for driving this discussion and putting a good draft together.

    Suggestions from my side:

    a) Suggest to describe “Here are a few guidelines with an example on how to structure your PR” as PR template – see https://help.github.com/articles/creating-a-pull-request-template-for-your-repository/. We should follow as appropriate the existing mxnet issue template.

    b) I really like http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/.  https://blog.github.com/2015-01-21-how-to-write-the-perfect-pull-request/ also has a lot of good suggestions we should consider. Maybe abstract and summarize the main points and include in this doc + reference.

    c) Suggest to reference (and we can use for training): https://blog.scottnonnenberg.com/top-ten-pull-request-review-mistakes/