Skip to content

Conversation

valeriy42
Copy link
Contributor

@valeriy42 valeriy42 commented Jul 2, 2020

This PR adds exponent aggregator required for the regression models trained with MSLE objective function.

Closes #1372

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.

Good catch! I think the comment needs updating and I'd like to make sure this fails to compile if we add a new loss function, but otherwise looks good.

//! Allows to use logistic regression aggregation.
//!
//! Given a weights vector $\vec{w}$ as a parameter and an output vector from the ensemble $\vec{x}$,
//! it computes the logistic regression function \f$1/(1 + \exp(-\vec{w}^T \vec{x}))\f$.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment looks wrong. The forest is predicting sum_i{ leaf_i } = log(1 + x), so for the weighted case the aggregator would be exp(w^t x) - 1 no?

Copy link
Contributor

@tveasey tveasey Jul 2, 2020

Choose a reason for hiding this comment

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

In fact, thinking about it, 1 is the offset parameter which we also need to supply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, the aggregator computes exp(w^t x) cf. CMsle::transform.

Copy link
Contributor

@tveasey tveasey Jul 2, 2020

Choose a reason for hiding this comment

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

Ok great.

And the comment on the offset was me forgetting what we're actually doing. For posterity, we compute leaf values by argmin_x (log(offset + exp(x)) - log(offset + actual))^2, i.e. the important part is that prediction is exponentiated so the forest is effectively predicting logs. The offset just governs how we penalise relative errors between the predictions and actuals at the scale of offset.

@valeriy42
Copy link
Contributor Author

Thank you @tveasey for the review. I addressed your comments. It would be great if you could take another look.

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.

LGTM

@valeriy42
Copy link
Contributor Author

retest

@valeriy42 valeriy42 merged commit 53fa223 into elastic:master Jul 6, 2020
@valeriy42 valeriy42 deleted the exponent-aggregator branch July 6, 2020 07:17
valeriy42 added a commit to valeriy42/ml-cpp that referenced this pull request Jul 6, 2020
…tic#1375)

This PR adds exponent aggregator required for the regression models trained with MSLE objective function.
valeriy42 added a commit that referenced this pull request Jul 6, 2020
… (#1383)

This PR adds exponent aggregator required for the regression models trained with MSLE objective function.

Backport of #1375.
valeriy42 added a commit to valeriy42/ml-cpp that referenced this pull request Jul 6, 2020
…tic#1375)

This PR adds exponent aggregator required for the regression models trained with MSLE objective function.
valeriy42 added a commit that referenced this pull request Jul 7, 2020
…1388)

This PR adds exponent aggregator required for the regression models trained with MSLE objective function.

Backport of #1375.
@droberts195 droberts195 added v7.8.1 and removed v7.8.2 labels Jul 9, 2020
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.

[ML] Inference models for MSLE regression trees should have exp aggregators
3 participants