Skip to content

[Python][ML] Fix and re-enable rdataloader xgboost tutorial - #23044

Merged
siliataider merged 4 commits into
root-project:masterfrom
siliataider:tmva-tutorial
Aug 7, 2026
Merged

[Python][ML] Fix and re-enable rdataloader xgboost tutorial#23044
siliataider merged 4 commits into
root-project:masterfrom
siliataider:tmva-tutorial

Conversation

@siliataider

@siliataider siliataider commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This Pull request:

Changes or fixes:

This PR fixes the ml_dataloader XGBoost tutorial, which had to be turned off because it crashed sporadically in CI, 2 separate issues are fixed here:

  • The previous way of splitting cluster boundaries for train/val counts was splitting every cluster independently using the validation split ratio, and counting on the last cluster to compensate for whatever rounding shortfall had built up from the earlier ones, if the last cluster didn't have enough entries, we sometimes ended up with less training entries than the actual batch size we promise, making us read past the rows that were actually written.
  • Separately, the "weights" column was handled differently: it was being handed to Python as a NumPy view into the C++ data, while the "features" and "target" columns were being handed over as copies. The view only stayed valid as long as the object that produced it was still alive, but that object didn't actually live long enough in this particular tutorial, as the generator we create goes out of scope at the end of the load_data function, so the weights view could end up pointing at memory that was already gone.
    • To fix this we made everything work the same way: features, target, and weights are now all views, and none of them depend on that short-lived object anymore. The data itself is moved out into its own independent object, so it survives for as long as the view into it is being used.
    • While working on this we made an interface change: columns previously required all training columns + targets + wieghts to be present in that list, it is no longer the case

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work investigating this! The changes look good overall but I left some minor comments related to commit and code structure

Comment thread tree/ml/inc/ROOT/ML/RFlat2DMatrix.hxx Outdated
Comment thread tree/ml/inc/ROOT/ML/RFlat2DMatrix.hxx
Comment thread bindings/pyroot/pythonizations/python/ROOT/_pythonization/_ml_dataloader.py Outdated
@vepadulano vepadulano closed this Aug 7, 2026
@vepadulano vepadulano reopened this Aug 7, 2026
@siliataider
siliataider force-pushed the tmva-tutorial branch 2 times, most recently from 37a07c0 to 0da11f0 Compare August 7, 2026 11:02
Previously, the `columns` keyword argument in RDataLoader's constructor
required all `target` and `weight` columns to also be listed explicitly.
This is no longer required, targets and weights can be specified independently
without needing to be duplicated in `columns`.
Additionally batches previously returned copies for features and targets
but views for weights. This has changes and RDataLoader now returns
views for all three.

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Test Results

    22 files      22 suites   3d 15h 28m 59s ⏱️
 3 857 tests  3 856 ✅ 0 💤 1 ❌
75 739 runs  75 735 ✅ 2 💤 2 ❌

For more details on these failures, see this check.

Results for commit aba9e2f.

@siliataider
siliataider merged commit 5138a61 into root-project:master Aug 7, 2026
30 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in:ML Everything under ROOT/ML in:Python Interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants