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

Add XGBoost JSON Reader/writer #461

Merged
merged 7 commits into from
Jul 16, 2024
Merged

Add XGBoost JSON Reader/writer #461

merged 7 commits into from
Jul 16, 2024

Conversation

stephenpardy
Copy link
Contributor

Related to #458


What

  • Adds ability to save and load models in xgboost's JSON format for better backwards compatibility

How to Test

  • Run unit tests

@stephenpardy stephenpardy marked this pull request as ready for review July 15, 2024 21:40
@stephenpardy stephenpardy requested review from a team as code owners July 15, 2024 21:40
@ryanSoley
Copy link
Member

a rebase should handle the test failures now

Copy link
Member

@ryanSoley ryanSoley left a comment

Choose a reason for hiding this comment

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

looks good! just one question

) -> Artifact:
"""Log an XGBoost model as a JSON file to this client object.

Please note that we do not currently support logging directly from the SKLearn interface.
Copy link
Member

Choose a reason for hiding this comment

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

what do you mean by this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If users use the XGBRegressor or XGBClassifier classes that implement sklearn interface for xgboost this will not work. They would have to use model.get_booster() first and log that. An obvious addition is to check for that ourselves and then call model.get_booster() if relevant.

I can add that in this PR if you want.

Copy link
Member

Choose a reason for hiding this comment

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

ahh gotcha, nah this is fine for now

@ryanSoley
Copy link
Member

closing and re-opening to try to get the license check to work

@ryanSoley ryanSoley closed this Jul 16, 2024
@ryanSoley ryanSoley reopened this Jul 16, 2024
@stephenpardy stephenpardy merged commit 878d691 into main Jul 16, 2024
11 checks passed
@stephenpardy stephenpardy deleted the xgboost branch July 16, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants