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

PUBDEV-7241 Monotone Quantile Constraints #4719

Merged
merged 13 commits into from
Sep 21, 2020

Conversation

maurever
Copy link
Contributor

@maurever maurever self-assigned this Jun 16, 2020
@maurever maurever marked this pull request as draft June 18, 2020 14:40
@maurever maurever force-pushed the maurever_PUBDEV-7241_quantile_monotone_constraints branch from 14bb8b7 to 4db7c54 Compare August 26, 2020 14:21
@maurever maurever changed the base branch from rel-zahradnik to rel-zeno August 26, 2020 14:21
@maurever maurever marked this pull request as ready for review September 1, 2020 16:38
_vals[_vals_dim*b + 0] += weight;
_vals[_vals_dim*b + 1] += wy;
_vals[_vals_dim*b + 2] += wyy;
int binDimStart = _vals_dim*b;
Copy link
Contributor

Choose a reason for hiding this comment

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

imho this reads worse than before, maybe if you made binDimStart final it would look better

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 I made the binDimStart final. Maybe this reads worse, however, it is better from a code perspective. But I can change it back if you want @michalkurka.

// left tree prediction quantile
if (bin <= b) {
if (bin == 1) {
ratio = 100 / wlo[b];
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the 100 number coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I calculated the percentile instead of quantile here. My mistake. I fixed it and run tests and demo and, it looks good. The calculation of the quantile is similar, so the performance should be the same.

@michalkurka
Copy link
Contributor

First read - LGTM, since you are touching performance optimized code in DTree - it would be a good idea to re-run benchmarks on this branch before merging this.

Copy link
Contributor

@honzasterba honzasterba left a comment

Choose a reason for hiding this comment

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

lgtm

@maurever maurever changed the title PUBDEV-7241 Monotone Quantile Constraints POC PUBDEV-7241 Monotone Quantile Constraints Sep 18, 2020
@maurever maurever force-pushed the maurever_PUBDEV-7241_quantile_monotone_constraints branch from 0537e0c to f630970 Compare September 18, 2020 14:17
@maurever maurever changed the base branch from rel-zeno to master September 18, 2020 14:17
@maurever maurever merged commit e1ab951 into master Sep 21, 2020
@maurever maurever deleted the maurever_PUBDEV-7241_quantile_monotone_constraints branch September 21, 2020 09:44
flaviusburca pushed a commit to mware-solutions/h2o-3 that referenced this pull request Apr 21, 2021
PUBDEV-7241
* implement histogram approximation quantile monotone constraints
* reset quantiles correctly with respect constraints in subtrees
* add quatnile to MC documentation, add demo
* add Java, Python and R tests

(cherry picked from commit e1ab951)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants