Remove nroll argument from rollout #2246
Merged
Conversation
This file contains 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
yuvaltassa
reviewed
Nov 25, 2024
doc/changelog.rst
Outdated
| @@ -56,20 +56,24 @@ MJX | |||
| 11. Added support for ``eq_active``. Fixes :github:issue:`2173`. | |||
| 12. Added ray intersection with ellipsoid. | |||
|
|
|||
| Python bindings | |||
| ^^^^^^^^^^^^^^^ | |||
| 13. Removed ``nroll`` argument from rollout because its value can always be implied | |||
There was a problem hiding this comment.
This is the wrong section? Should be at the top of the file "Upcoming version (not yet released)"
There was a problem hiding this comment.
Whoops, my apologies.
yuvaltassa
reviewed
Nov 25, 2024
doc/changelog.rst
Outdated
| @@ -18,6 +18,7 @@ MJX | |||
| Python bindings | |||
| ^^^^^^^^^^^^^^^ | |||
| - Provide prebuilt wheels for Python 3.13. | |||
| - Removed ``nroll`` argument from rollout because its value can always be implied | |||
There was a problem hiding this comment.
from rollout -> from :ref:rollout<PyRollout>
(apologies for the markdown, please look at the source of this comment)
There was a problem hiding this comment.
I'm asking to convert the word "rollout" to an RST link
There was a problem hiding this comment.
Like here
Line 558 in e9a2f05
yuvaltassa
reviewed
Nov 25, 2024
aftersomemath
force-pushed
the
remove-nroll
branch
from
49c08c1 to
d45d30b
Compare
|
One test was added for each argument that can be used to infer |
yuvaltassa
reviewed
Nov 25, 2024
doc/changelog.rst
Outdated
| @@ -18,6 +18,7 @@ MJX | |||
| Python bindings | |||
| ^^^^^^^^^^^^^^^ | |||
| - Provide prebuilt wheels for Python 3.13. | |||
| - Removed ``nroll`` argument from :ref:`rollout<PyRollout>` because its value can always be implied | |||
There was a problem hiding this comment.
- implied -> inferred
- Add full stop
|
Please sync to latest |
nroll can always be implied from the other arguments of rollout. Add tests for nroll inference.
aftersomemath
force-pushed
the
remove-nroll
branch
from
d45d30b to
96a8f00
Compare
|
Squashed and rebased |
yuvaltassa
approved these changes
Nov 25, 2024
copybara-service
bot
merged commit 6e129a5
into
google-deepmind:main
13 of 14 checks passed
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.
nrollcan always be implied from the other arguments of rollout.I grepped the codebase and did not find any cases where
nrollwas being passed to rollout other than its threading test.