[python-package] do not copy column-major numpy arrays when creating Dataset

[jmoralez]

Changes the logic to flatten the data array to consider its layout:

• If the array is Fortran contiguous, then it's flattened as such and passed to the Dataset constructor indicating that the array is column-major, which avoids copying it to make it row-major.
• If the array is C contiguous, no copy is made (current behavior).
• If the array is not contiguous because it was sliced, then a copy is made to make it C-contiguous (current behavior).

I ran some quick tests with an array of (100k, 50) and the timings to construct the dataset from a C-contiguous and an F-contiguous arrays are roughly the same, so this doesn't introduce extra latency, just avoids the copy.

[jmoralez]

This could also be done for the predict portion, i.e.

LightGBM/python-package/lightgbm/basic.py

Lines 1268 to 1307 in 5151fe8

	def __inner_predict_np2d(
	self,
	mat: np.ndarray,
	start_iteration: int,
	num_iteration: int,
	predict_type: int,
	preds: Optional[np.ndarray],
	) -> Tuple[np.ndarray, int]:
	if mat.dtype == np.float32 or mat.dtype == np.float64:
	data = np.asarray(mat.reshape(mat.size), dtype=mat.dtype)
	else: # change non-float data to float data, need to copy
	data = np.array(mat.reshape(mat.size), dtype=np.float32)
	ptr_data, type_ptr_data, _ = _c_float_array(data)
	n_preds = self.__get_num_preds(
	start_iteration=start_iteration,
	num_iteration=num_iteration,
	nrow=mat.shape[0],
	predict_type=predict_type,
	)
	if preds is None:
	preds = np.empty(n_preds, dtype=np.float64)
	elif len(preds.shape) != 1 or len(preds) != n_preds:
	raise ValueError("Wrong length of pre-allocated predict array")
	out_num_preds = ctypes.c_int64(0)
	_safe_call(
	_LIB.LGBM_BoosterPredictForMat(
	self._handle,
	ptr_data,
	ctypes.c_int(type_ptr_data),
	ctypes.c_int32(mat.shape[0]),
	ctypes.c_int32(mat.shape[1]),
	ctypes.c_int(_C_API_IS_ROW_MAJOR),
	ctypes.c_int(predict_type),
	ctypes.c_int(start_iteration),
	ctypes.c_int(num_iteration),
	_c_str(self.pred_parameter),
	ctypes.byref(out_num_preds),
	preds.ctypes.data_as(ctypes.POINTER(ctypes.c_double)),
	)
	)

Please let me know if it's ok to include it here or in a separate PR.

[StrikerRUS]

LGTM! Thank you for very useful improvement!

I just left a couple of non-blocking comments below for your consideration.

[StrikerRUS]

Just want to highlight that arrays can be C-contiguous and F-contiguous simultaneously.
https://stackoverflow.com/q/60230562

Arrays can be both C-style and Fortran-style contiguous simultaneously. This is clear for 1-dimensional arrays, but can also be true for higher dimensional arrays.
https://numpy.org/doc/stable/reference/generated/numpy.ndarray.flags.html

Seems it's correctly handled here.

[StrikerRUS]

Please let me know if it's ok to include it here or in a separate PR.

I believe it would be better to have a follow-up PR for prediction after merging this one.

[jameslamb]

This looks great to me! I agree with all of @StrikerRUS 's comments, so won't approve until those are addressed. But I don't have any additional comments.

Thanks for working on this, and really nice tests!

[StrikerRUS]

As this test now doesn't include train(), I think it should go to test_basic.py file.

[jmoralez]

Sorry. Moved in 38c6786

[StrikerRUS]

@jameslamb

I agree with all of @StrikerRUS 's comments, so won't approve until those are addressed.

Would you like to take a look after the recent commits?

[jameslamb]

Changes look great to me, thank you!

I looked at the failing Dask tests... they're unrelated to this PR. Documented in #6739.

@jmoralez do you have time in the next few days to investigate that?

[jmoralez]

Sure, I'll take a look.

[github-actions]

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.