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

Add a hyper-parameter tuning module #1101

Merged
merged 34 commits into from Sep 5, 2017

Conversation

Projects
None yet
3 participants
@micyril
Member

micyril commented Aug 25, 2017

See the previous discussion here.

micyril added some commits Jul 26, 2017

Add assertion for argument types of Optimize
The added assertion causes a compilation error if one of aguments passed
to the Optimize method of HyperParameterTuner is neither of an
arithmetic type, nor of a collection type, nor fixed with the Fixed
function.

micyril added some commits Aug 27, 2017

@micyril

This comment has been minimized.

Show comment
Hide comment
@micyril

micyril Aug 28, 2017

Member

@rcurtin: when I was rebasing the branch to the master branch, there were a lot of conflicts with the commit "Refactor DatasetMapper to map any input type". So, please review this particular commit.

Also, I have noticed that the style checks haven't been run for my last submission. Is it expected behavior?

Member

micyril commented Aug 28, 2017

@rcurtin: when I was rebasing the branch to the master branch, there were a lot of conflicts with the commit "Refactor DatasetMapper to map any input type". So, please review this particular commit.

Also, I have noticed that the style checks haven't been run for my last submission. Is it expected behavior?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 28, 2017

Member

@mlpack-jenkins style-check this

Member

zoq commented Aug 28, 2017

@mlpack-jenkins style-check this

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 28, 2017

Member

Not sure, why the last commit did not trigger the style check job. But we can always ask @mlpack-jenkins to run the check.

Member

zoq commented Aug 28, 2017

Not sure, why the last commit did not trigger the style check job. But we can always ask @mlpack-jenkins to run the check.

@rcurtin

Looks great to me. Thanks again for the hard work. Let's wait for 5 days before merge, just to make sure everyone who wants to comment has a chance.

The DatasetMapper changes look fine to me; sorry that the bimap change caused trouble for you at merge. I think that maybe someday it might be worthwhile to refactor so that InputType is a function template parameter to the mapping functions, instead of a class template parameter, but I don't think that needs to happen today, that can be later.

@zoq

zoq approved these changes Sep 1, 2017

Thanks for letting this sit for a couple days. The code is really well written; just made a minor comment about the default HyperParameterTuner parameter.

MatType,
PredictionsType,
WeightsType>::HyperParameterTuner(const CVArgs&... args) :
cv(args...), relativeDelta(0.01), minDelta(1e-10) {}

This comment has been minimized.

@zoq

zoq Sep 1, 2017

Member

Should we provide a default constructor and set the default vales there? That way doxygen can pick up the default values and we can also add a note that the values are not necessarily good for the given problem, and suggest to adjust each to the task at hand. Let me know what you think.

@zoq

zoq Sep 1, 2017

Member

Should we provide a default constructor and set the default vales there? That way doxygen can pick up the default values and we can also add a note that the values are not necessarily good for the given problem, and suggest to adjust each to the task at hand. Let me know what you think.

This comment has been minimized.

@micyril

micyril Sep 2, 2017

Member

It is problematic since we need to accept variadic template parameters in the constructor. I thought about mentioning the default values in the documentation for the getters and setters, but I hadn't seen anything like that in the mlpack codebase. Let me know if you think it's ok to do that.

@micyril

micyril Sep 2, 2017

Member

It is problematic since we need to accept variadic template parameters in the constructor. I thought about mentioning the default values in the documentation for the getters and setters, but I hadn't seen anything like that in the mlpack codebase. Let me know if you think it's ok to do that.

This comment has been minimized.

@zoq

zoq Sep 2, 2017

Member

Right I missed that, I think you could add a note to the class description itself, but adding it to the getter/setter documentation is also fine; it's up to you.

@zoq

zoq Sep 2, 2017

Member

Right I missed that, I think you could add a note to the class description itself, but adding it to the getter/setter documentation is also fine; it's up to you.

@rcurtin rcurtin merged commit f8d659b into mlpack:master Sep 5, 2017

4 checks passed

Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Sep 5, 2017

Member

Thanks for the great contribution. :)

Member

rcurtin commented Sep 5, 2017

Thanks for the great contribution. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment