search

JuliaAI / MLJXGBoostInterface.jl

MIT License
11 stars 5 forks source link

fix cases where watchlist can be improperly passed as nothing #29

Closed ExpandingMan closed 1 year ago

ExpandingMan commented 1 year ago

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 as nothing (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 the watchlist argument seems to cause a lot of confusion.

ExpandingMan commented 1 year ago

This should also now have fixed a much worse issue in which a fix to the predict output in XGBoost.jl was resulting in mangled output here. This is because I had originally worked around that issue here. How I'm such an idiot that I did all this without realizing it had to be comprehensively fixed I don't know. Definitely in part due to my not taking classification seriously enough.

ExpandingMan commented 1 year ago

Not going to waste any time merging and tagging this since the issues were pretty egregious.

ablaom commented 1 year ago

Not going to waste any time merging and tagging this since the issues were pretty egregious.

Not sure I understand. Are you saying the issue is too important to wait for a review?

ablaom commented 1 year ago

Sounds like we may need some extra tests to catch what we had appeared to overlooked prior to this PR?

ExpandingMan commented 1 year ago

Are you saying the issue is too important to wait for a review?

Just that there was basically no way it was going to be worse off than before. :shrug:

Sorry if this was overzealous. Tell you what, I'll wait for you to merge from now on.

ExpandingMan commented 1 year ago

Sounds like we may need some extra tests to catch what we had appeared to overlooked prior to this PR?

The real problem was in XGBoost.jl and tests were added.

ablaom commented 1 year ago

Sorry if this was overzealous.

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.

ablaom commented 1 year ago

Please go ahead and tag a new release, it looks good to me.

ExpandingMan commented 1 year ago

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.