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

Tests are broken after update to libxgboost #43

Closed
rikhuijzer opened this issue Oct 6, 2023 · 2 comments · Fixed by #44
Closed

Tests are broken after update to libxgboost #43

rikhuijzer opened this issue Oct 6, 2023 · 2 comments · Fixed by #44

Comments

@rikhuijzer
Copy link
Member

rikhuijzer commented Oct 6, 2023

I'm quickly opening this issue as a reminder for us. I'm currently already inside a fix inside a fix, so I'll just open this issue and focus on my main project for the day again.

The tests here are broken after the release of XGBoost 2.4 (dmlc/XGBoost.jl#191). Came up in #42.

@tylerjthomas9
Copy link
Contributor

tylerjthomas9 commented Oct 7, 2023

I think the change to hist as the default tree method in libxgboost v2 is causing the test failures. Changing the default tree_method="exact" causes all the tests to pass. The histogram approach is just giving slightly worse performance on the test data. It is probably worth just bumping these thresholds for test failures.

@ExpandingMan
Copy link
Collaborator

Ah, I made a comment on the PR before I read this. I'm a little surprised that they made a change to libxgboost that violated the tests since I surely believed my bounds were rather generous at the time, so I must say I feel slightly puzzled, however, as I said in my other comment, those bounds were surely set empirically based on no more than ~10 tests I did in my specific environment, so if everyone feels it's appropriate to loosen them I'm happy to go along with that.

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 a pull request may close this issue.

3 participants