NotificationsYou must be signed in to change notification settings
Fork352
Star6.6k

Added random_state as an available hyper parameter for xgboost models#659

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

montanalow merged 1 commit intopostgresml:masterfromSilasMarvin:set-random-seed-for-xgboost

May 29, 2023

Merged

Added random_state as an available hyper parameter for xgboost models#659

montanalow merged 1 commit intopostgresml:masterfromSilasMarvin:set-random-seed-for-xgboost

May 29, 2023

Conversation

Copy link

Contributor

SilasMarvin commentedMay 29, 2023

I followed the standard found throughout the repo and just calledunwrap on line 166. This does have the potential to cause somewhat confusing error messages if they have malformed arguments. I would be happy to go through and replace theunwrap() calls withexpect(SOME HELPFUL ERROR MESSAGE) throughout pgml-extension if that would be helpful?

Added random_state as an available hyper parameter for xgboost models

ac76d8d

montanalow approved these changes

May 29, 2023

View reviewed changes

Copy link

Contributor

montanalow commentedMay 29, 2023

You're right.expect() is preferable tounwrap(), but rather than trying to convert all existing cases, I think it'd be better to try to move most functions that currentlyunwrap to instead return ananyhow::Result, and addcontext() to their calls for better nested function call clarity.

I think the currenttransformer bindings fortransform andembed are the worst offenders with the mostly likely failure cases, and the scikit algorithm's are probably second worst on that front.