Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine AbstractModule methods #2262

Merged
merged 18 commits into from Feb 9, 2018

Conversation

yiheng
Copy link
Contributor

@yiheng yiheng commented Feb 1, 2018

What changes were proposed in this pull request?

This PR is code refactor, include:

  1. make getParamter method as final and private[bigdl]. That method should only used in BigDL optimization. If user accident call it outside, the optimization result maybe incorrect
  2. Remove updateParameters method. It's only used for debug, should not be a public API. User should use optimizer instead of updateParameter method to change the model weights
  3. ZeroGrad should be final, remove the redundant implementations
  4. add final to necessary methods
  5. reorder the methods, put public method first

How was this patch tested?

unit test

Related links or issues (optional)

fixed #966

@zhichao-li
Copy link
Contributor

One idea is we can provide a common method for getGrad, then zeroGrad doent's need to be override in every place as it just zero grads....

*
* This function will go over all the weights and gradWeights and make them view into a single
* tensor (one for weights and one for gradWeights). Since the storage of every weight and
* gradWeight is changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this sentence is not finished.

@yiheng
Copy link
Contributor Author

yiheng commented Feb 5, 2018

@zhichao-li This PR is removing the unnecessary zeroGrad methods. ZeroGrad is using parameter() method to get the grads. We needn't an extra getGrad methods

@yiheng yiheng changed the title Refine parameters related method Refine AbstractModule methods Feb 7, 2018
@yiheng
Copy link
Contributor Author

yiheng commented Feb 7, 2018

jenkins test pass

Validator(this, dataSet).test(vMethods)
}

def quantize(): Module[T] = {
/**
* Quantize this module, whcih reduces the precison of the parameter. Get a higher speed with a
Copy link
Contributor

@yangw1234 yangw1234 Feb 7, 2018

Choose a reason for hiding this comment

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

"which" and "precision" are spelled wrong.

@yangw1234
Copy link
Contributor

LGTM

@yiheng
Copy link
Contributor Author

yiheng commented Feb 9, 2018

jenkins pass.merge

@yiheng yiheng merged commit 77ad6c0 into intel-analytics:master Feb 9, 2018
@yiheng yiheng deleted the refine_get_parameters branch February 14, 2018 02:05
psyyz10 pushed a commit to psyyz10/BigDL that referenced this pull request Mar 8, 2018
* make getParameter be final and private

* remove updateParameter Method

* remove useless override zeroGrad

* fix comments

* fix unit tests

* fix unit tests

* allocate gradWeight storage if it's not allocated

* fix unit test

* meet code review

* make zeroGrad become final

* fix compile error

* add final to module apis

* add private[bigdl] to some module methods

* reorder the method sequence

* meet code review

* remove unnecessary getParameterTable and fix unit test

* fix unit test

* fix unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enrich function doc for getParameters()
3 participants