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

[R] Enable booster slicing #9948

Merged
merged 1 commit into from
Jan 20, 2024
Merged

Conversation

david-cortes
Copy link
Contributor

ref #9810

To be rebased after PR #9924 is merged, as it contains the commits from it.

This PR adds a wrapper over the C function to slice booster rounds.

The slicing logic is made in such a way that it maches with R's syntax for sequences, but there were some issues from the C-level function as described in previous issue: #9944

so the tests aren't very comprehensive

@trivialfis
Copy link
Member

apologies for the difficulty of stacking PRs, I will try to look into #9924 as soon as possible.

@@ -62,6 +64,7 @@ export(xgb.plot.tree)
export(xgb.save)
export(xgb.save.raw)
export(xgb.set.config)
export(xgb.slice.Booster)
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed after having the s3 method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed, but I think it helps with discoverability compared to having just an S3 method.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for sharing, sounds good!

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! Can merge once rebase is done.

@david-cortes
Copy link
Contributor Author

david-cortes commented Jan 20, 2024

Sorry - I'm not sure what happened here. Looks like the PR somehow closed itself after force-pushing, and shows it has being merged already, which it isn't. Looks like I cannot reopen it from here either.

EDIT: figured out I had pushed the wrong commit. Now it should be good.

@david-cortes david-cortes reopened this Jan 20, 2024
@trivialfis trivialfis merged commit 5062a3a into dmlc:master Jan 20, 2024
45 of 49 checks passed
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