Skip to content

Base XGBoost's shuffle warning on the fit loader, not the dataset flag - #1313

Open
solarsys wants to merge 1 commit into
sunlabuiuc:masterfrom
solarsys:fix/xgboost-stale-shuffle-warning
Open

solarsys wants to merge 1 commit into
sunlabuiuc:masterfrom
solarsys:fix/xgboost-stale-shuffle-warning

Conversation

@solarsys

Copy link
Copy Markdown
Collaborator

Problem

XGBoostModel.fit warns when its training loader shuffles, because row order changes row subsampling. The check reads loader.dataset.shuffle. Loaders from get_dataloader (_SeededDataLoader) set that flag on the shared dataset at the start of each pass. So after any shuffled loader on the same dataset has run, for example another model's training loader, the flag stays True. fit with get_dataloader(ds, ..., shuffle=False) then warns that the loader shuffles, which is false.

This showed up in the new Tutorial 00, which trains an RNN and XGBoost on the same split. The notebook currently works around it by training XGBoost first.

Fix

Use the loader's own shuffle_samples setting when it has one. That is what applies when the loader iterates. Other loaders keep the old check: a RandomSampler, or the dataset's flag.

Test

test_shuffle_warning_follows_the_fit_loader: after iterating a shuffled loader on the dataset, fitting with an unshuffled loader logs no warning, and fitting with a shuffled loader still warns. It fails without the fix. All 22 tests in tests/core/test_xgboost_model.py pass, ruff is clean, and tools/check_pr_rules.py passes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PhbzTePoLMBYwjMLG8R9ek

get_dataloader's loaders set the dataset's shuffle flag at the start of
each pass, so after another model trains with a shuffled loader on the
same dataset, the flag stays True. XGBoostModel.fit then warned that an
unshuffled loader shuffles. Read the loader's own setting when it has
one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PhbzTePoLMBYwjMLG8R9ek
@github-actions

Copy link
Copy Markdown

Test Coverage

Total: 67.87% (16694 / 24597 lines)

Modified file Coverage Lines
pyhealth/models/gradient_boosted_trees.py 27.86% 78 / 280

Workflow run · f334f34

This branch has not been deployed

No deployments
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.

1 participant