-
Notifications
You must be signed in to change notification settings - Fork 4k
[python-package] Add test for converting a ctypes int64 pointer array to a NumPy array
#7071
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
base: master
Are you sure you want to change the base?
Conversation
ctypes int64 pointer array to a NumPy arrayctypes int64 pointer array to a NumPy array
jameslamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @nicklamiller
Before I review this... did you try to do this through LightGBM's public API?
From #7031:
Tests should only use lightgbm's public API, unless that is very difficult or expensive. Any function whose name begins with a _ is considered private.
This particular internal function is so small and simple, I think it'd be preferable to have tests which cover it via the public API. That'd give us more coverage of lightgbm under whatever conditions lead to this function being invoked. I'm not exactly sure what code paths do that, probably passing int64 arrays for label or something,... you'd have to do a bit of investigation.
This reverts commit 511070f.
@jameslamb thank you very much for the detailed instructions in #7031 and sorry for completely missing that point! Testing through the public API to mimic how functionality is called in the wild by users makes sense, and Newly covered linesRunning: pytest \
--cov=lightgbm \
--cov-report="term" \
--cov-report="html:htmlcov" \
tests/python_package_test/test_engine.pyand viewing coverage report for Some additional notes/thoughts:
`_cint64_array_to_numpy` call chain from Booster(...).predict(...)
Booster.predict(pred_contrib=True)
↓
_InnerPredictor.predict(pred_contrib=True)
↓
__pred_for_csr(csr, predict_type=_C_API_PREDICT_CONTRIB)
↓
__inner_predict_csr_sparse(csr, predict_type=_C_API_PREDICT_CONTRIB)
↓
_LIB.LGBM_BoosterPredictSparseOutput() # C API call
↓
__create_sparse_native(csr, out_ptr_indptr, ...)
↓
_cint64_array_to_numpy(cptr=out_ptr_indptr, length=indptr_len) |


Contributes to: #7031
Adds test for
_cint64_array_to_numpy:LightGBM/python-package/lightgbm/basic.py
Lines 504 to 509 in 5dbfcdc