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

[TMVA] API-Support for SGD Optimizer: #2309

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

ravikiran0606
Copy link
Contributor

@ravikiran0606 ravikiran0606 commented Jul 10, 2018

[TMVA] API-Support for SGD Optimizer:

  • Add Base Class VOptimizer.
  • Add Derived Class TSGD with Momentum implementation.
  • Add Unit Tests for SGD Optimizer.
  • Modify the MethodDL Tests to include parsing options for Optimizer.

An example Training Strategy string may look like,

"LearningRate=1e-1,Optimizer=SGD,Momentum=0.9,Repetitions=1,"
"ConvergenceSteps=20,BatchSize=256,TestRepetitions=10,"
"WeightDecay=1e-4,Regularization=L2,"
"DropConfig=0.0+0.5+0.5+0.5, Multithreading=True"

Reference Implementation: Tensorflow
Blog Post: https://www.sravikiran.com/GSOC18//2018/07/09/sgd/

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@ravikiran0606 ravikiran0606 changed the title SGD Optimizer API Support for SGD Optimizer Jul 10, 2018
@stwunsch
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on slc6/gcc48, slc6-i686/gcc49, centos7/clang39, centos7/gcc62, centos7/gcc7, fedora28/native, ubuntu16/native, mac1013/native, windows10/vc15 with flags -Dimt=ON -Dccache=ON
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on centos7/clang39.
See console output.

Warnings:

  • ../root/core/metacling/src/TClingValue.cxx:51:25: warning: comparison of integers of different signs: 'int' and 'size_type' (aka 'unsigned long') [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:92:23: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:95:26: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:105:26: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:92:23: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:95:26: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:105:26: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:92:23: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:95:26: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  • include/TMVA/DNN/SGD.h:105:26: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]

And 3 more

Failing tests:

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@phsft-bot
Copy link
Collaborator

Starting build on slc6/gcc48, slc6-i686/gcc49, centos7/clang39, centos7/gcc62, centos7/gcc7, fedora28/native, ubuntu16/native, mac1013/native, windows10/vc15 with flags -Dimt=ON -Dccache=ON
How to customize builds

@stwunsch
Copy link
Contributor

stwunsch commented Jul 11, 2018

you should see the warnings regarding the comparisons of unsigned long and int as well on your system. can you check? the tests for all builds seem to be fine. is it possible for you to access cdash.cern.ch?

as well, please rebase and squash your commits in a single commit with a meaningful message.

last change: please put a [TMVA] tag at the begin of your commit message so that it's clear what you've worked on in the commit history.

@ravikiran0606
Copy link
Contributor Author

@phsft-bot build

@ravikiran0606 ravikiran0606 changed the title API Support for SGD Optimizer [TMVA] API-Support for SGD Optimizer: Jul 11, 2018
@stwunsch
Copy link
Contributor

you cannot trigger the PR builds ;) have you fixed the warnings due to the type comparisons?

@stwunsch
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on slc6/gcc48, slc6-i686/gcc49, centos7/clang39, centos7/gcc62, centos7/gcc7, fedora28/native, ubuntu16/native, mac1013/native, windows10/vc15 with flags -Dimt=ON -Dccache=ON
How to customize builds

@ravikiran0606
Copy link
Contributor Author

@stwunsch Yes, I have fixed the warnings with type comparisons and squashed the commits into a single commit.

@phsft-bot
Copy link
Collaborator

Build failed on centos7/gcc62.
See console output.

Warnings:

  • ../root/core/metacling/src/TClingValue.cxx:51:25: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on centos7/gcc7.
See console output.

Warnings:

  • ../root/core/metacling/src/TClingValue.cxx:51:25: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  • ../root/core/newdelete/src/NewDelete.cxx:217:11: warning: ‘operator new’ must not return NULL unless it is declared ‘throw()’ (or -fcheck-new is in effect)
  • ../root/core/newdelete/src/NewDelete.cxx:344:11: warning: ‘operator new’ must not return NULL unless it is declared ‘throw()’ (or -fcheck-new is in effect)
  • ../root/core/newdelete/src/NewDelete.cxx:263:6: warning: the program should also define ‘void operator delete(void*, std::size_t)’ [-Wsized-deallocation]
  • ../root/core/newdelete/src/NewDelete.cxx:367:6: warning: the program should also define ‘void operator delete [](void*, std::size_t)’ [-Wsized-deallocation]

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on slc6/gcc48.
See console output.

Warnings:

  • ../root/core/metacling/src/TClingValue.cxx:51:39: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Failing tests:

@stwunsch
Copy link
Contributor

@lmoneta i think that we are good to merge. can you check?

@@ -76,6 +76,11 @@ enum class EInitialization {
kGlorotUniform = 'F',
};

/* Enum representing the optimizer used for training. */
Copy link
Member

Choose a reason for hiding this comment

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

Can you use Doxygen style comments: /// Enum representing the optimizer used for training.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used this way of commenting to ensure consistency since other enums in the same file were using this style for commenting. So should I need to change this?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm bad and totally not your fault! Please use a style that at least doxygen-documents yours.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this.

inline void IncrementGlobalStep() { this->fGlobalStep++; }

/*! Getters */
inline Scalar_t GetLearningRate() { return fLearningRate; }
Copy link
Member

Choose a reason for hiding this comment

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

Several of these should be const!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

void Step();

/*! Virtual Destructor. */
virtual ~VOptimizer();
Copy link
Member

Choose a reason for hiding this comment

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

Please use virtual ~VOptimizer() = default;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

typename DeepNet_t = TDeepNet<Architecture_t, Layer_t>>
class VOptimizer {
using Matrix_t = typename Architecture_t::Matrix_t;
using Scalar_t = typename Architecture_t::Scalar_t;
Copy link
Member

Choose a reason for hiding this comment

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

These are private using declarations, but they are used by public interfaces. Could you make these types public, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

/*! Getters */
inline Scalar_t GetLearningRate() { return fLearningRate; }
inline size_t GetGlobalStep() { return fGlobalStep; }
inline std::vector<Layer_t *> GetLayers() { return fDeepNet.GetLayers(); }
Copy link
Member

Choose a reason for hiding this comment

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

Do you really want to copy the vector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm! I changed that.

break;
default:
optimizer = std::unique_ptr<DNN::TSGD<Architecture_t, Layer_t, DeepNet_t>>(
new DNN::TSGD<Architecture_t, Layer_t, DeepNet_t>(settings.learningRate, deepNet, settings.momentum));
Copy link
Member

Choose a reason for hiding this comment

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

As this case is identical with the previous, please combine them, adding a commend /*Intentional fall-through*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So Shall I remove the default case and have only one case for EOptimizer::kSGD for now?

Copy link
Member

Choose a reason for hiding this comment

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

I don't care; maybe @lmoneta has an opinion. But having a second, default case for a single-valued enum is - surprising.

// copy configuration when reached a minimum error
if (testError < minTestError ) {
// Copy weights from deepNet to fNet
Log() << std::setw(10) << stepCount << " Minimun Test error found - save the configuration " << Endl;
Log() << std::setw(10) << optimizer->GetGlobalStep()
Copy link
Member

Choose a reason for hiding this comment

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

Note that from here on, every one else writing to Log() (which defaults to std::cerr or std::cout?) will have "suffer" from the setw(10) call. Please consider capturing the flags and resetting them afterwards. See e.g. https://stackoverflow.com/a/2273352/6182509

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was like this previously. I haven't changed that part of the code. And I am not able to get what change I want to do here?

Copy link
Member

Choose a reason for hiding this comment

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

OK fine - we'll fix it when people complain!

* (http://tmva.sourceforge.net/LICENSE) *
**********************************************************************************/

#ifndef TMVA_TEST_DNN_TEST_OPTIMIZER_H
Copy link
Member

Choose a reason for hiding this comment

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

Can you match the file name, i.e. ...TEST_OPTIMIZATION_H (not OPTIMIZER)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, Fixed.

if (error > 1e-5) {
return 1;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add a trailing newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm! Fixed.

std::cout << "Stochastic Gradient Descent with momentum: Maximum relative error = " << error << std::endl;
if (error > 1e-5) {
return 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

You're missing a crucial return 0! This shouldn't succeed without... Did you run this test and it worked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! The test worked. Anyways I'll add this.

@ravikiran0606
Copy link
Contributor Author

@stwunsch @lmoneta I have made changes as said by @Axel-Naumann. Can you please review them?

@stwunsch
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on slc6/gcc48, slc6-i686/gcc49, centos7/clang39, centos7/gcc62, centos7/gcc7, fedora28/native, ubuntu16/native, mac1013/native, windows10/vc15 with flags -Dimt=ON -Dccache=ON
How to customize builds

@stwunsch
Copy link
Contributor

@ravikiran0606 clang-format does not seem to be happy with the formatting of the file headers. can you check?

@stwunsch
Copy link
Contributor

PR builds without errors or warnings, nice!

* Implement Base Class VOptimizer

* Implement Class TSGD with Momentum method

* Add Unit Tests for TSGD

* Add Parsing Options for Optimizers

* Modify MethodDL to include the optimizer TSGD

* Add ROOT Style docs and clang-format the code
@stwunsch
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on slc6/gcc48, slc6-i686/gcc49, centos7/clang39, centos7/gcc62, centos7/gcc7, fedora28/native, ubuntu16/native, mac1013/native, windows10/vc15 with flags -Dimt=ON -Dccache=ON
How to customize builds

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

Why the Optimizer class needs to be templated on the Layer type and the network ?
Would not be enough to be templated only on the Architecture ?

@ravikiran0606
Copy link
Contributor Author

@stwunsch @lmoneta As we discussed, the three template arguments are needed because I use them inside the class definition of the VOptimizer class. I actually thought of doing so by getting ideas from the DeepNet class, which has two template arguments, one for Architecture and another for Layer. So I did use a similar approach.

@ravikiran0606
Copy link
Contributor Author

ravikiran0606 commented Jul 19, 2018

@stwunsch I have also updated the PR with the clang formatting. Now the Travis ci build succeds with clang format option too. I guess now we are good to merge! :D

@stwunsch stwunsch merged commit 5bf9b81 into root-project:master Jul 19, 2018
@ravikiran0606 ravikiran0606 deleted the SGD-Optimizer branch July 25, 2018 04:59
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.

None yet

5 participants