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 SingleRowPredictor::IsPredictorEqual comparison (invert) #2799

Merged
merged 1 commit into from
Feb 24, 2020

Conversation

AlbertoEAF
Copy link
Contributor

@AlbertoEAF AlbertoEAF commented Feb 22, 2020

This fixes the issue #2798.

As stated in that issue, this should fix the bug in case the parameters change, and improve performance when they don't.

Please give me feedback if I'm doing anything in a less than desirable way.

@guolinke
Copy link
Collaborator

ping @imatiach-msft to confirm

@imatiach-msft
Copy link
Contributor

@AlbertoEAF great catch, this should improve performance during scoring. The main bugfix that introduced this bug was changing the correctness from:
(single_row_predictor_.get() == nullptr)
to:
single_row_predictor_[predict_type].get() == nullptr
so that when calling raw predict and predict for probabilities the function would return the correct values, but it looks like now we are not re-using the cached predictor which is horrible. Sorry I introduced this performance issue after fixing the correctness issue before.

"worse, not creating a new one if conditions change"
I don't think this can happen from someone using mmlspark which is calling those APIs (since the model is immutable after it is trained) but this is definitely something that could help anyone who is using the native API directly, although I'm not aware of anyone other than mmlspark using LGBM_BoosterPredictForMatSingleRow and LGBM_BoosterPredictForCSRSingleRow. Regardless this is a great fix, and thank you for sending it out!

Copy link
Contributor

@imatiach-msft imatiach-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch!

@imatiach-msft
Copy link
Contributor

also adding @eisber for context who added the original optimizations, it looks like his optimizations of improving scoring by 3-4X are missing in latest mmlspark

@AlbertoEAF
Copy link
Contributor Author

AlbertoEAF commented Feb 24, 2020

Thank you @matiach-smft :)

Regarding no one else using the native API, there might be in the next months :p. If I can help a bit this project great, I'd like to learn and help more :)

@guolinke guolinke added the fix label Mar 1, 2020
@AlbertoEAF AlbertoEAF deleted the fix/c-api-IsPredictorEqual branch March 21, 2020 12:14
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants