Fix cudf.pandas failure on test_convert_matrix_order_cuml_array
#5882
Conversation
…rix_order_cuml_array
|
Just confirmed that this PR indeed fixes the |
|
I think the test stopped failing. Out of interest, how did you check it stopped failing (in CI)? I opened https://github.com/rapidsai/cuml/actions/runs/8976060393/job/24678633926?pr=5882 and looked at the output/scrolled to the end of the output and then searched for |
|
Making sure I understand the change: you added an explicit step to the input handling that changes the order for pandas. As a result it now does the same thing as cudf. Which means we don't need two separate |
| # by default pandas converts to numpy 'C' major, which | ||
| # does not keep the original order | ||
| if order == "K": | ||
| X = X.reshape(X.shape, order="F") |
There was a problem hiding this comment.
I didn't know that you could change the order via reshape and was wondering whether or not this leads to a copy of the data. So I played around a bit to see it happen with my own eyes.
df = pd.DataFrame({'a': [1,2,3], 'b':[11,22,33]})
X = df.to_numpy()
print(X.flags) # C_CONTIGUOUS is false, F_CONTIGUOUS is true
X = X.reshape(X.shape, order="F")
print(X.flags) # C_CONTIGUOUS is false, F_CONTIGUOUS is true :-/Which left my puzzled. Is my assumption of what should be happening wrong? There is np.asfortranarray() which does change C to F, but at the cost of making a copy of the data.
There was a problem hiding this comment.
Ok, got myself confused. For me the result of to_numpy() is already F contiguous?!
There was a problem hiding this comment.
seems like the result can be inconsistent depending on the wrapping that cudf is doing
There was a problem hiding this comment.
That said, it should have no cost if it's already in the correct order
Error came from the fact that pandas and cudf convert to numpy by default with different order.
Towards fixing #5876