Skip to content

ntreelimit has been deprecated in favour of iteration_range #656

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
Feb 16, 2022
Merged

ntreelimit has been deprecated in favour of iteration_range #656

merged 8 commits into from
Feb 16, 2022

Conversation

tiagomaie
Copy link
Contributor

This is a fix for issue #655

As ntreelimit as been deprecated, adding iterationrange to the
xgb_pred call with the correct modifications to fit the current use case.

Further work is needed at a later stage to extend the new functionality of iterationrange.

As `ntreelimit` as been deprecated, adding `iterationrange` to the
`xgb_pred` call with the correct modifications to fit the current use case.
@juliasilge
Copy link
Member

Do we want to make this change now and set xgboost (>= 1.5.0.1) in DESCRIPTION? Or try to handle both old/new versions somehow?

As a reminder, xgboost is only in Suggests for parsnip but how would this impact our ability to build on older versions of R? We are building back to oldrel-2 right now.

@topepo
Copy link
Member

topepo commented Feb 15, 2022

xgboost uses R (≥ 3.3.0) so it probably shouldn't been an issue for CI.

I"m inclined to add a version restriction in parsnip. The xgboost api does change a lot and programming around this (and other changes) would be messy.

@juliasilge
Copy link
Member

juliasilge commented Feb 16, 2022

@tiagomaie I just want to confirm with you that I changed the parameter to iteration_range based on the NEWS and docs (from what you submitted in your PR here, iterationrange).

@tiagomaie
Copy link
Contributor Author

Thanks @juliasilge ! I should mention that although in some warnings and in the docs it says that iteration_range should be used, I think these stem from implementations in other languages, like python.
On my end (and also looking through xgboost R code base and documentation, see predict.xgb.Booster for example) it seems that the correct parameter and the one that produces the correct behavior is iterationrange.

Running parsnips test for xgboost (ie. devtools::test(filter='boost_tree_xgboost')), fail to produce the correct result if iteration_range is used instead of iterationrange in xgb_pred. From my understanding, this is because predict.xgb.Booster won't take it as a parameter and instead will just assign the ntreelimit with the value of 0.

Co-authored-by: Tiago Maié <tiagomaie@hotmail.com>
@juliasilge
Copy link
Member

Ah, thank you very much for clarifying @tiagomaie! Very helpful.

@juliasilge juliasilge merged commit 45ec8e6 into tidymodels:main Feb 16, 2022
@tiagomaie tiagomaie deleted the xgboost_deprecated_arg_fix branch February 17, 2022 10:01
@github-actions
Copy link

github-actions bot commented Mar 4, 2022

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants