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

[MXNET-1340, 1339][Fit API][WIP]Multi input/output #14628

Closed
wants to merge 23 commits into from
Closed

[MXNET-1340, 1339][Fit API][WIP]Multi input/output #14628

wants to merge 23 commits into from

Conversation

roywei
Copy link
Member

@roywei roywei commented Apr 5, 2019 •
edited
Loading

Description

To address feedback described in issues
MXNET-1339 Multi input and output support
MXNET-1340 Improve estimator train stats, net, trainer initialization logic
MXNET-1367 Improve validation

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:
  • 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

  • break estimator intialization into different components
  • add FitHelper for multi input and output
  • add total step counter for more granular controls, for example checkpoint and stop based on number of steps trained
  • make validation an event handler

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

roywei and others added 21 commits March 15, 2019 22:16
@roywei @nswamy
[MXNet-1334][Fit API]base class for estimator and eventhandler (apach…
41392fa 
…e#14346)

* base class for estimator and eventhandler

* add license

* add event handlers

* fix pylint

* improve arg check

* fix pylint

* add unit tests
@piyushghai @wkcn
Fixed issue where the estimator was printing beyond the dataset size … (
e6c63b1 
apache#14464)

* Fixed issue where the estimator was printing beyond the dataset size for the last batch

* Added comments

* Nudge to CI
@roywei
add train history
d71eba9
@roywei
update history
a8c2c7f
@abhinavs95 @nswamy
[MXNet-1349][Fit API]Add validation support and unit tests for fit() …
8186772 
…API (apache#14442)

* added estimator unittests

* add more tests for estimator

* added validation logic

* added error handlers, unittests

* improve val stats

* fix pylint

* fix pylint

* update unit test

* fix tests

* fix tests

* updated metrics, val logic

* trigger ci

* trigger ci

* update metric, batch_fn error handler

* update context logic, add default metric
@roywei
update test
29c68b4
@roywei
merge with fit-api
ad4041b
@roywei
avoid calling empty methods
b8ec43c
@roywei
remove train history object
904cdc7
@roywei
fix pylint
c6fe873
@roywei
add unit test
6e690ca
@roywei
fix test
55b102e
@roywei
update categorize handlers
feac6e3
@roywei @nswamy
[MXNet-1340][Fit API]Update train stats (apache#14494)
ed7f6e5 
* add train history

* update history

* update test

* avoid calling empty methods

* remove train history object

* fix pylint

* add unit test

* fix test

* update categorize handlers
@karan6181 @nswamy
[MXNet-1375][Fit API]Added RNN integration test for fit() API (apache…
81ec379 
…#14547)

* Added RNN integration test for fit() API

* Addressed review comments: change in JenkinFile, tmp directory, ctx with condense if/else, renamed imports

* CPU test doesn't require nvidiadocker container

* Modified the structure by removing the redundant code
@roywei
merge remote/fit-api
ca2dd76
@abhinavs95 @nswamy
[MXNet-1343][Fit API]Add CNN integration test for fit() API (apache#1…
b1ef99a 
…4405)

* added cnn intg tests for fit api

* updated cnn intg tests

* added functions for nightly test

* updated runtime_function

* updated intg tests

* updated init, datapath, refs

* added validation data

* update cpu test

* refactor code

* updated context
@roywei
add fit helper
5cc5585
@roywei
update fit helper
ba1718a
@roywei
merge with upstream
c5f48a6
@roywei
refractor estimator init
8d503ae
@roywei roywei requested a review from szha as a code owner April 5, 2019 16:52
@roywei roywei mentioned this pull request Apr 5, 2019
Merged
5 tasks
@piyushghai
Copy link
Contributor

Thanks for your contributions @roywei. There seem to be merge conflicts in this PR. Can you have a look ?

@mxnet-label-bot Add [Gluon, pr-awaiting-review]

@marcoabreu marcoabreu added Gluon pr-awaiting-review PR is waiting for code review labels Apr 5, 2019
@Roshrini
Copy link
Member

@roywei Can you take a look at build failures. There are few indentation errors as well.

@roywei roywei mentioned this pull request Apr 19, 2019
Merged
7 tasks
@roywei roywei changed the title [MXNET-1340, 1339][Fit API]Multi input/output [MXNET-1340, 1339][Fit API][WIP]Multi input/output Apr 24, 2019
@roywei
Copy link
Member Author

roywei commented Apr 24, 2019

closing this PR fow now since it depends on the finalized Estimator and event handler. Will create PR directly to master once #14629 is finalized and merged.

@roywei roywei closed this Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@szha szha Awaiting requested review from szha

Assignees
No one assigned
Labels
Gluon 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.
6 participants
@roywei @piyushghai @Roshrini @marcoabreu @abhinavs95 @karan6181