[Draft] [R] Add QuantileDMatrix creation from dense matrices #9864

david-cortes · 2023-12-10T19:30:26Z

This PR adds a wrapper over QuantileDMatrix for dense matrix inputs.

I am not very sure that I'm implementing all of this correctly so would be ideal if a maintainer could take a deep look.

I have some doubts about how this class (QuantileDMatrix) is meant to be created:

Should feature types / names / etc. be added to the proxy dmatrix as part of the iterations?
Should labels and/or bounds be set as part of the iterations? If not, are categorical features supported for this class?
If the call to XGQuantileDMatrixCreateFromCallback fails, returning a non-zero code, does the proxy dmatrix still need to be freed manually through XGDMatrixFree?
Is there something that would currently work with a regular DMatrix but not with a QuantileDMatrix, and which would need to be checked on the R side instead of passing things to the C functions?

Also a small note: there's a PR which hasn't yet been merged as it's currently failing MSVC jobs, which introduces some refactorings around the handling of 'missing' parameter in the JSON configs. After such refactoring is introduced, it should also be used as part of the code introduced here, as otherwise missing values in integer matrices will not be handled correctly.

trivialfis · 2023-12-11T01:05:39Z

Let me briefly answer some the of questions first, then try to come up with improved documents.

Should feature types / names / etc. be added to the proxy dmatrix as part of the iterations?

yes.

Should labels and/or bounds be set as part of the iterations? If not, are categorical features supported for this class?

yes, it should be part of the iterations. Yes, categorical features are supported.

If the call to XGQuantileDMatrixCreateFromCallback fails, returning a non-zero code, does the proxy dmatrix still need to be freed manually through XGDMatrixFree?

Yes, everything created successfully should be freed accordingly. If the proxy is created successfully, it should be freed, regardless of how it's used.

Is there something that would currently work with a regular DMatrix but not with a QuantileDMatrix,

The QDM works with the hist tree method (default) for training and normal prediction. It's used to conserve memory for training.

and which would need to be checked on the R side instead of passing things to the C functions?

I usually just let the C functions emit errors.

trivialfis · 2023-12-11T01:08:41Z

R-package/R/xgb.DMatrix.R

+  feature_weights = NULL,
+  as_quantile_dmatrix = FALSE,
+  ref = NULL,
+  max_bin = NULL


It's probably better to split it into a sub class.

Yes, since the code paths will be entirely different, better to have a dedicated function xgb.QuantileDMatrix like in the python interface.

david-cortes · 2023-12-11T18:48:16Z

Now I'm confused.

If I understand it correctly, creating a QuantileDMatrix from a dense input involves setting the proxy dmatrix 4 times.

From some quick tests, it seems to be possible to pass label by calling XGDMatrixSetInfoFromInterface after the QuantileDMatrix has been created with argument data only (at least for cases without categorical data).

If the label were to be set on the proxy dmatrix at every iterator call, wouldn't that result in more work being done compared to setting it after? Aren't there conversions and memory copies happening inside those calls?

Also, just to be sure - if one is supposed to set the feature names and types on the proxy dmatrix at every iterator call, does that mean that one can pass different subsets of columns in different iterations?

By the way, what would happen in a case in which XGQuantileDMatrixCreateFromCallback returns an error code, and then XGDMatrixFree after that also returns an error code? Can that happen? How/what should be reported to the user then?

trivialfis · 2023-12-12T00:36:25Z

Let me write some documents for the QDM internal, it's a special case for the iterator interface, which is also used by external memory support. As a result, some of the things seem redundant, but we kept it to simplify the code so that it can work with both use cases without any special handling. So, to answer the question of whether it can accept a subset of samples, yes, but it's for external memory support and #9864 (comment).

By the way, what would happen in a case in which XGQuantileDMatrixCreateFromCallback returns an error code, and then XGDMatrixFree after that also returns an error code? Can that happen?

If you are trying to free the proxy, then should succeed assuming the proxy was successfully created. If you are trying to free the QDM, then it should fail.

How/what should be reported to the user then?

If FromCallback returns -1, then it's likely that something went wrong during iteration, which means an error in the next or reset function. In this case:

Report the errors from callbacks. No need to free the unsuccessfully created QDM, (no harm in doing it either since it will deterministically fail anyway). See:

xgboost/src/c_api/c_api.cc

Line 323 in b3700bb

*out = new std::shared_ptr<xgboost::DMatrix>{
xgboost/src/c_api/c_api.cc

Line 312 in b3700bb

xgboost::DMatrix::Create(iter, proxy, reset, next, missing, n_threads, cache)};

, if the Create static method fails, there's no memory leak since there's no pointer being allocated.

If no error from reset/next callbacks, then report the error from FromCallback as usual like the original DMatrix class.

trivialfis · 2023-12-12T00:38:56Z

it can accept a subset of samples, yes, but it's for external memory support.

And yes, the QDM indeed can take on batches on initialization. See quantile_data_iterator.py in demo. It's lesser known but can be helpful when a user doesn't have the memory capacity to construct the QDM while holding the original data in memory.

trivialfis · 2023-12-12T00:43:05Z

Ref: #9734 (comment)

Would it be possible to have C functions to create these QuantileDMatrices from array_interface strings for dense/csr data sources + required parameters? Is there some limitation that would make such an approach not work?

it can no longer take on iterators as described in #9864 (comment).

R-package/R/xgb.DMatrix.R

trivialfis · 2023-12-13T06:38:00Z

R-package/src/xgboost_R.cc

+XGB_DLL SEXP XGQuantileDMatrixFromMat_R(SEXP R_mat, SEXP missing, SEXP n_threads,
+                                        SEXP max_bin, SEXP ref_dmat) {
+  SEXP ret = PROTECT(R_MakeExternalPtr(nullptr, R_NilValue, R_NilValue));
+  R_API_BEGIN();
+  DMatrixHandle proxy_dmat_handle;
+  CHECK_CALL(XGProxyDMatrixCreate(&proxy_dmat_handle));
+  DMatrixHandle out_dmat;
+  int res_code1, res_code2;
+
+  try {
+    xgboost::Json jconfig{xgboost::Object{}};
+    /* FIXME: this 'missing' field should have R_NaInt when the input is an integer matrix. */
+    jconfig["missing"] = Rf_asReal(missing);
+    if (!Rf_isNull(n_threads)) {
+      jconfig["nthread"] = Rf_asInteger(n_threads);
+    }
+    if (!Rf_isNull(max_bin)) {
+      jconfig["max_bin"] = Rf_asInteger(max_bin);
+    }
+    std::string json_str = xgboost::Json::Dump(jconfig);
+
+    DMatrixHandle ref_dmat_handle = nullptr;
+    if (!Rf_isNull(ref_dmat)) {
+      ref_dmat_handle = R_ExternalPtrAddr(ref_dmat);
+    }
+
+    std::string array_str = MakeArrayInterfaceFromRMat(R_mat);
+    _RMatrixSingleIterator single_iterator(proxy_dmat_handle, array_str.c_str());
+
+    res_code1 = XGQuantileDMatrixCreateFromCallback(
+      &single_iterator,
+      proxy_dmat_handle,
+      ref_dmat_handle,
+      _reset_RMatrixSingleIterator,
+      _next_RMatrixSingleIterator,
+      json_str.c_str(),
+      &out_dmat);
+    res_code2 = XGDMatrixFree(proxy_dmat_handle);
+  } catch(IteratorError &err) {
+    XGDMatrixFree(proxy_dmat_handle);
+    Rf_error(XGBGetLastError());
+  }
+
+  CHECK_CALL(res_code2);
+  CHECK_CALL(res_code1);
+
+  R_SetExternalPtrAddr(ret, out_dmat);
+  R_RegisterCFinalizerEx(ret, _DMatrixFinalizer, TRUE);
+  R_API_END();
+
+  UNPROTECT(1);
+  return ret;
+}


Depending on your roadmap, it might be desirable to implement this in R instead of C++, the data iterator is the common interface to QDM and external memory, I will share some old unpublished documents soon.

I think it'd be better to use a dedicated C++-only route for single-iteration QuantileDMatrix, and then later on implement a customizable DataIterator in R.

trivialfis · 2023-12-18T15:09:53Z

Is this still draft?

david-cortes · 2023-12-18T15:28:39Z

Is this still draft?

Yes, and I'm going to create a new MR as I see this kind of functionality would need to be coded differently.

david-cortes · 2023-12-20T19:52:16Z

Closing in favor of bigger PR that follows the same approach as in the python interface: #9913

david-cortes added 2 commits December 10, 2023 20:15

add QuantileDMatrix creation from R dense matrices

c1221d9

add static qualifier for non-exported functions

c0d2212

trivialfis reviewed Dec 11, 2023

View reviewed changes

trivialfis mentioned this pull request Dec 12, 2023

Proposal for new R interface (discussion thread) #9734

Open

solve merge conflicts

a5288be

trivialfis reviewed Dec 13, 2023

View reviewed changes

solve merge conflicts

8eb062d

david-cortes mentioned this pull request Dec 20, 2023

[R] Add data iterator, quantile dmatrix, and missing feature_types #9913

Merged

david-cortes closed this Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] [R] Add QuantileDMatrix creation from dense matrices #9864

[Draft] [R] Add QuantileDMatrix creation from dense matrices #9864

david-cortes commented Dec 10, 2023 •

edited

Loading

trivialfis commented Dec 11, 2023

trivialfis Dec 11, 2023

david-cortes Dec 13, 2023

david-cortes commented Dec 11, 2023

trivialfis commented Dec 12, 2023 •

edited

Loading

trivialfis commented Dec 12, 2023

trivialfis commented Dec 12, 2023

trivialfis Dec 13, 2023

david-cortes Dec 13, 2023

trivialfis commented Dec 18, 2023

david-cortes commented Dec 18, 2023

david-cortes commented Dec 20, 2023

[Draft] [R] Add QuantileDMatrix creation from dense matrices #9864

[Draft] [R] Add QuantileDMatrix creation from dense matrices #9864

Conversation

david-cortes commented Dec 10, 2023 • edited Loading

trivialfis commented Dec 11, 2023

trivialfis Dec 11, 2023

Choose a reason for hiding this comment

david-cortes Dec 13, 2023

Choose a reason for hiding this comment

david-cortes commented Dec 11, 2023

trivialfis commented Dec 12, 2023 • edited Loading

trivialfis commented Dec 12, 2023

trivialfis commented Dec 12, 2023

trivialfis Dec 13, 2023

Choose a reason for hiding this comment

david-cortes Dec 13, 2023

Choose a reason for hiding this comment

trivialfis commented Dec 18, 2023

david-cortes commented Dec 18, 2023

david-cortes commented Dec 20, 2023

david-cortes commented Dec 10, 2023 •

edited

Loading

trivialfis commented Dec 12, 2023 •

edited

Loading