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

[R] Use array interface for dense DMatrix creation #9816

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

david-cortes
Copy link
Contributor

@david-cortes david-cortes commented Nov 28, 2023

ref #9810

This PR switches the underlying C function for DMatrix creation from dense R matrices to the XGDMatrixCreateFromDense function used in the python interface.

Along the way, it also fixes a potential memory leak in case of failures, in which a failure to allocate an R externalptr would lead to a longjump without freeing the allocated C++ object from its pointer.

I see there was already some mechanism to create the kind of array JSON that numpy uses in the DMatrix creators from sparse matrices, but couldn't find any equivalent for dense matrices. There also seems to be a dedicated module to create JSONs, but they look simple enough for snprintf.

The JSON parser for the config by the way seems to have issues with nan and inf, so this PR makes a conversion to NaN and Infinity which seem to be accepted.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on the dense matrix, let me make some changes tomorrow for using the JSON class in XGBoost.

@trivialfis
Copy link
Member

Hi @david-cortes , I have made some changes to use the linalg module in XGBoost for generating the strings, could you please take a look when you are available? I'm open to suggestions and feel free to ping me if you find some of the code confusing.

@david-cortes
Copy link
Contributor Author

Hi @david-cortes , I have made some changes to use the linalg module in XGBoost for generating the strings, could you please take a look when you are available? I'm open to suggestions and feel free to ping me if you find some of the code confusing.

Thanks for the changes. I've made some very small modifications on top and I guess it should be good to go now.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Will merge once the CI is green.

@trivialfis trivialfis merged commit 37da66f into dmlc:master Nov 29, 2023
25 of 26 checks passed
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.

2 participants