Repository navigation
fix: make bar distribution reductions device-agnostic - #1366
aniruddhaadak80 wants to merge 2 commits into
Conversation
`FullSupportBarDistribution.mean` re-homes its buffer onto the logits'
device; `median`, `mode` and `icdf` index `borders` with logit-derived
indices and did not. A criterion sitting on a different device than the
logits therefore failed every reduction except `mean`, with
RuntimeError: indices should be either on cpu or on the same device
as the indexed tensor
That is reachable whenever a fitted estimator is reassembled and only
part of it is placed — restoring one and moving the weights but not the
bar distribution, for instance. `mean` silently working made it look
like a partial failure rather than a placement problem.
Index a same-device view of the borders in `icdf` and `mode`, matching
what `mean` already does. `quantile` and `ucb` inherit it via `icdf`.
`bucket_widths` derives from the borders, so it follows.
The `.to()` is a no-op when the devices already match: results are
bit-identical on the co-located path, and match to float32 tolerance
across devices.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit afb1eb2. Configure here.
| `borders` directly and raised. That can happen whenever an estimator is | ||
| reassembled and only part of it is moved — e.g. restoring a fitted regressor | ||
| and placing the weights but not the bar distribution. | ||
| """ |
There was a problem hiding this comment.
Test docstring narrates prior failure
Low Severity
The module docstring recounts what previous code did and why it raised, including that mean already re-homed its buffer while median, mode, and icdf indexed borders directly. Docstrings should state current behavior only, not the incident or prior implementation.
Reviewed by Cursor Bugbot for commit afb1eb2. Configure here.
|
Hi @aniruddhaadak80, thanks for the time you've put into TabPFN. A few of your earlier fixes were useful and we merged them.We've since adopted contribution guidelines, and your open issues and PRs don't fit them. External contributors can have at most 3 open PRs, and you have 10. The AI section also asks that a human reviews every submission before it goes in, answers review questions personally, and only reports problems they actually ran into. Bulk-filed reports from automated bug-hunting are called out as not welcome. Oscar's question on #1317 never got an answer.So we're closing all of your open issues and PRs. We'll fix #1344 and #1355 ourselves and credit you in the commits.If you want to keep contributing, please open one focused PR at a time that you've reviewed yourself, and reply to reviewers in your own words. If you think we got something wrong, reply here. |
Supersedes local branch fix/bardist-device-agnostic-reductions. See commit history for details.