-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix cases where watchlist can be improperly passed as nothing #29
Conversation
This should also now have fixed a much worse issue in which a fix to the |
Not going to waste any time merging and tagging this since the issues were pretty egregious. |
Not sure I understand. |
Sounds like we may need some extra tests to catch what we had appeared to overlooked prior to this PR? |
Just that there was basically no way it was going to be worse off than before. 🤷 Sorry if this was overzealous. Tell you what, I'll wait for you to merge from now on. |
The real problem was in XGBoost.jl and tests were added. |
I'm not too bothered. I do really appreciate you're taking on some much of the maintenance of this package (and XGBoost.jl). However, I'm still happy and generally available to provide reviews, which I think is helpful and good practice. |
Please go ahead and tag a new release, it looks good to me. |
Ok, I did so earlier. Yeah, I agree, it's not a good practice, I was just really eager to get if fixed as I was really bothered by it. I'll refrain from doing it in the future. |
There are some shenanigans going on to check the
watchlist
argument to the model, and prior to this PR in some cases it would get passed asnothing
(an invalid value). This should fix #28. I think this ought to be further improved but at the moment it's not clear to me what a "good" design would be here. Might also call for changes in XGBoost.jl since thewatchlist
argument seems to cause a lot of confusion.