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

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

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

ExpandingMan
Copy link
Collaborator

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
Copy link
Collaborator Author

ExpandingMan commented Jan 9, 2023

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
Copy link
Collaborator Author

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

@ablaom
Copy link
Member

ablaom commented Jan 9, 2023

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
Copy link
Member

ablaom commented Jan 9, 2023

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

@ExpandingMan
Copy link
Collaborator Author

ExpandingMan commented Jan 9, 2023

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. 🤷

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

@ExpandingMan
Copy link
Collaborator Author

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
Copy link
Member

ablaom commented Jan 9, 2023

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
Copy link
Member

ablaom commented Jan 9, 2023

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

@ExpandingMan
Copy link
Collaborator Author

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.

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.

Issue when verbosity > 0 and a test does not pass no my Mac
2 participants