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

XGBoost: Fix type mismatch for CSR conversion in c_api #3194

Merged
merged 5 commits into from Oct 27, 2022

Conversation

yzhliu
Copy link
Contributor

@yzhliu yzhliu commented Oct 20, 2022

Description

There was a mismatch between bst_ulong in c_api.h and ctypes.c_size_t in python. If you run test_scipy_sparse in browser, it would trigger error like,

TypeError: Cannot convert 10 to a BigInt
    at ffi_call (pyodide.asm.js:10:189569)
    at pyodide.asm.wasm:0x276cba
    at pyodide.asm.wasm:0x270f9c
    at method_call_trampoline (pyodide.asm.js:10:218037)
    at pyodide.asm.wasm:0x126317
    at pyodide.asm.wasm:0x1e246f
    at pyodide.asm.wasm:0x1e00b3
    at pyodide.asm.wasm:0x1da6a5
    at pyodide.asm.wasm:0x126a07
    at pyodide.asm.wasm:0x1e248c
API.fatal_error @ pyodide.asm.js:10
pyodide.asm.js:10 Stack (most recent call first):
pyodide.asm.js:10   File "/lib/python3.10/site-packages/xgboost/data.py", line 87 in _from_scipy_csr
pyodide.asm.js:10   File "<ipython-input-9-f40ece33f18d>", line 1 in <module>
pyodide.asm.js:10   File "/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 3378 in run_code
...

In which 10 is the ncol.

I've also created an upstream PR dmlc/xgboost#8369

Checklists

@ryanking13
Copy link
Member

Thanks a lot @yzhliu! It looks good to me. Let's wait for a review in your upstream PR. (Feel free to ping me when it's accepted)

@yzhliu
Copy link
Contributor Author

yzhliu commented Oct 26, 2022

Hi @ryanking13 thanks for the review. the upstream PR has been merged, I have also updated this PR per their suggestion. Let me if this is good to go.

@ryanking13
Copy link
Member

Thanks a lot @yzhliu!

@ryanking13 ryanking13 merged commit ee77f6c into pyodide:main Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants