-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[tests][python] Handle data types more accurate in C API test #4297
Conversation
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.
Looks good! Thanks very much for breaking long lines like into one-argument-per-line. I think that makes it easier to read, which is really important since some of the functions take so many positional arguments.
I just left one question about how we're using ctypes
but it isn't directly related to this PR and could be addressed in a later PR if something needs to change.
c_array(ctypes.c_int, csr.indices), | ||
csr.data.ctypes.data_as(ctypes.POINTER(ctypes.c_void_p)), | ||
dtype_float64, | ||
csr.indptr.ctypes.data_as(ctypes.POINTER(ctypes.c_int32)), |
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.
could this use .byref()
instead of POINTER()
?
From https://docs.python.org/3/library/ctypes.html#passing-pointers-or-passing-parameters-by-reference
ctypes exports the
byref()
function which is used to pass parameters by reference. The same effect can be achieved with thepointer()
function, although pointer() does a lot more work since it constructs a real pointer object, so it is faster to usebyref()
if you don’t need the pointer object in Python itself.
I'm very unfamiliar with ctypes
so apologies if that doesn't make sense.
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.
I wish I knew... The cited paragraph looks like we can benefit from using byref
instead of pointer
. I will create a new issue for this idea to not lose in the merged PR and read about it.
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.
Ok thanks very much!
label = np.array(label, dtype=np.float32) | ||
csr = sparse.csc_matrix(mat) | ||
csc = sparse.csc_matrix(mat) |
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 catching this!
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. |
Linking #4292.
Match function signatures from https://github.com/microsoft/LightGBM/blob/master/include/LightGBM/c_api.h.