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

Fast single row predict API v2 #3268

Merged

Conversation

AlbertoEAF
Copy link
Contributor

@AlbertoEAF AlbertoEAF commented Aug 1, 2020

@AlbertoEAF
Copy link
Contributor Author

AlbertoEAF commented Aug 1, 2020

@imatiach-msft @StrikerRUS @guolinke please review, it is now even faster by the latest tests.

It could be slightly faster if we used a fixed address for the input prediction data instance, but @imatiach-msft needs a dynamic one, so I left the code like this as the extra speedup was not that large.

Thank you for waiting for the patch and good luck with the v3 release! ;)

@AlbertoEAF AlbertoEAF force-pushed the fast-single-row-predict-api-v2 branch from c0b19cd to c5e59ea Compare August 1, 2020 23:37
@StrikerRUS StrikerRUS added the fix label Aug 2, 2020
@StrikerRUS
Copy link
Collaborator

I'm confused by const/non-const params mismatch in C API functions.

@guolinke
Copy link
Collaborator

guolinke commented Aug 4, 2020

For the non pointer arguments (and non reference$), I think we can unify them to the non constant types.

@AlbertoEAF
Copy link
Contributor Author

@StrikerRUS good catch! I had const mismatches even between the .c and .h in the function signatures which made things extremely weird.

@guolinke I put instead everything const just because many were lacking const when they could be. Now I think it's really easy to interpret from the user's viewpoint. Only the input booster - which will be modified - and output pointers, are non-const.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@AlbertoEAF Thanks a lot! I think with all const it looks much better.

@AlbertoEAF
Copy link
Contributor Author

@guolinke do you think it's ready to merge?

Thank you

@guolinke
Copy link
Collaborator

guolinke commented Aug 5, 2020

Yeah, looks good to me

@guolinke guolinke merged commit b5027de into microsoft:master Aug 5, 2020
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
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.

[bug] Introduced bug in *Fast() methods with the locking strategy PR
3 participants