Skip to content

Conversation

valeriy42
Copy link
Contributor

@valeriy42 valeriy42 commented Apr 24, 2020

This PR implements Pseudo-Huber loss function and integrates it into the RegressionRunner. Since it has a parameter, I needed to reimplement the persist and restore functionality in order to be able to save the state of the loss functions (the same functionality is useful for MSLE and multiclass classification). I also did some refactoring of unit tests avoid code duplication.

Relates to #973

@valeriy42 valeriy42 removed the WIP label Apr 29, 2020
Copy link
Contributor Author

@valeriy42 valeriy42 left a comment

Choose a reason for hiding this comment

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

first pass by myself

@valeriy42 valeriy42 requested a review from tveasey April 29, 2020 15:55
Copy link
Contributor

@tveasey tveasey left a comment

Choose a reason for hiding this comment

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

I've done a first pass. Basically looks great (and good job on tidying up restoring the loss while you were in the area)! I'd just like to have a more thorough read of the test code before approving, but pretty close.

Copy link
Contributor

@tveasey tveasey left a comment

Choose a reason for hiding this comment

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

I made a couple of suggestions in the tests. I think the main problem is going to be ensuring that the test data is positive: maybe just add on a largish constant?

Copy link
Contributor

@tveasey tveasey left a comment

Choose a reason for hiding this comment

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

Just a couple of points got missed, but no need to review any more. Good work!

@valeriy42 valeriy42 merged commit 1bd1f88 into elastic:master May 5, 2020
tveasey pushed a commit to tveasey/ml-cpp-1 that referenced this pull request May 5, 2020
This PR implements Pseudo-Huber loss function and integrates it into the RegressionRunner. Since it has a parameter, I needed to reimplement the persist and restore functionality in order to be able to save the state of the loss functions (the same functionality is useful for MSLE and multiclass classification). I also did some refactoring of unit tests avoid code duplication.
@droberts195 droberts195 mentioned this pull request May 5, 2020
valeriy42 added a commit to valeriy42/ml-cpp that referenced this pull request May 6, 2020
This PR implements Pseudo-Huber loss function and integrates it into the RegressionRunner. Since it has a parameter, I needed to reimplement the persist and restore functionality in order to be able to save the state of the loss functions (the same functionality is useful for MSLE and multiclass classification). I also did some refactoring of unit tests avoid code duplication.
valeriy42 added a commit that referenced this pull request May 6, 2020
* [ML] Pseudo-Huber loss function (#1168)

This PR implements Pseudo-Huber loss function and integrates it into the RegressionRunner. Since it has a parameter, I needed to reimplement the persist and restore functionality in order to be able to save the state of the loss functions (the same functionality is useful for MSLE and multiclass classification). I also did some refactoring of unit tests avoid code duplication.

* Fix test threshold (#1195)

The unit test CBoostedTreeTest/testPiecewiseConstant was
failing on Linux only.  This PR adjusts the test threshold
slightly.

Co-authored-by: David Roberts <dave.roberts@elastic.co>
valeriy42 added a commit that referenced this pull request May 6, 2020
* [ML] Pseudo-Huber loss function (#1168)

This PR implements Pseudo-Huber loss function and integrates it into the RegressionRunner. Since it has a parameter, I needed to reimplement the persist and restore functionality in order to be able to save the state of the loss functions (the same functionality is useful for MSLE and multiclass classification). I also did some refactoring of unit tests avoid code duplication.

* Fix test threshold (#1195)

The unit test CBoostedTreeTest/testPiecewiseConstant was
failing on Linux only.  This PR adjusts the test threshold
slightly.

Co-authored-by: David Roberts <dave.roberts@elastic.co>
valeriy42 added a commit that referenced this pull request May 8, 2020
While adding the additional function parameter in #1168, I wired it in the constructor of the MSLE loss function, but not in the computation of the objective. This PR fixes this, it basically substitutes log(1+x) by log(offset+x) in many different places.

I mark it as a non-issue since the MSLE loss function was not released yet.
valeriy42 added a commit to valeriy42/ml-cpp that referenced this pull request May 8, 2020
While adding the additional function parameter in elastic#1168, I wired it in the constructor of the MSLE loss function, but not in the computation of the objective. This PR fixes this, it basically substitutes log(1+x) by log(offset+x) in many different places.

I mark it as a non-issue since the MSLE loss function was not released yet.
valeriy42 added a commit to valeriy42/ml-cpp that referenced this pull request May 8, 2020
While adding the additional function parameter in elastic#1168, I wired it in the constructor of the MSLE loss function, but not in the computation of the objective. This PR fixes this, it basically substitutes log(1+x) by log(offset+x) in many different places.

I mark it as a non-issue since the MSLE loss function was not released yet.
valeriy42 added a commit that referenced this pull request May 11, 2020
While adding the additional function parameter in #1168, I wired it in the constructor of the MSLE loss function, but not in the computation of the objective. This PR fixes this, it basically substitutes log(1+x) by log(offset+x) in many different places.

I mark it as a non-issue since the MSLE loss function was not released yet.
valeriy42 added a commit that referenced this pull request May 11, 2020
While adding the additional function parameter in #1168, I wired it in the constructor of the MSLE loss function, but not in the computation of the objective. This PR fixes this, it basically substitutes log(1+x) by log(offset+x) in many different places.

I mark it as a non-issue since the MSLE loss function was not released yet.
@valeriy42 valeriy42 deleted the huber-loss-function branch June 8, 2020 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants