Repository navigation
[BugFix] Keep categorical modes one-hot when probabilities tie - #4509
Open
AHMETHAKANBEZIR1 wants to merge 1 commit into
Open
AHMETHAKANBEZIR1 wants to merge 1 commit into
AHMETHAKANBEZIR1 wants to merge 1 commit into
Conversation
Co-authored-by: Codex <noreply@openai.com>
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/4509
Note: Links to docs will display an error until the docs builds have been completed.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #4508 (assigned to AHMETHAKANBEZIR1 through the repository's
/assignworkflow).Use
argmaxfollowed byone_hotforOneHotCategorical.modeand the dense-maskMaskedOneHotCategorical.modepath. Comparing every value with the maximum previously returned multi-hot vectors when maxima tied, including uniform initial policies.deterministic_sampleandOneHotOrdinalinherit the corrected behavior.The existing sparse-mask mode path remains intact, preserving original action-index remapping and its first-maximum behavior when valid indices are unsorted.
Regression coverage
Hard-coded expected action indices provide the regression reference; existing sampling, log-probability and gradient tests are retained. CPU/CUDA parametrization uses the existing
get_default_devicesconvention; only CPU was available locally.Validation
8f92a50cb1e8543d6d7545605e4e5b399224928b.pytest test/test_distributions.py -k 'OneHotCategorical or MaskedCategorical or Ordinal' -q— 162 passed, including the two full-graph eager-backend compile checks.pre-commit run --all-files— all hooks passed with the repository's pinned versions. A separate Python 3.9 tooling environment andPYTHONUTF8=1were used for Windows compatibility.git diff --check— passed.Runtime: Windows, Python 3.12.14, PyTorch 2.10.0+cpu, TensorDict 0.14.2. TorchRL Python source is imported from the checkout. The optional C++ extension was not built; its absence warning is reported, and these distribution tests do not use it.
CUDA, Inductor, cudagraphs, full repository tests and the full documentation build were not run locally. The sparse-mask mode branch is unchanged; its existing super-property lookup was not claimed to be fully graph-capturable on this PyTorch version. An initial draft delegating all modes through
super().modecaused a Dynamo graph break on the changed paths, so the final fix keeps their direct logits-based computation.Checklist
No new API/signature is introduced. Codex assistance with implementation, investigation and testing is disclosed here and in the commit. The contributor reports having completed the Meta CLA; the repository's automatic verification remains separate.