Carry the dtype of a Discrete space through a round trip - #346
Open
DenisDrobyshev wants to merge 2 commits into
Open
DenisDrobyshev wants to merge 2 commits into
DenisDrobyshev wants to merge 2 commits into
Conversation
The serializer wrote "int64" for every Discrete space and the deserializer
ignored the field, so a space built with another dtype came back as a
different space:
space = spaces.Discrete(5, dtype=np.int32)
deserialize_space(serialize_space(space)) == space # False, dtype int64
Discrete only gained a dtype argument in Gymnasium 1.3.0 and this package
supports 0.28.1 upwards, so the argument is passed from 1.3.0 onwards and a
dataset that needs a narrower dtype on an older Gymnasium is reported rather
than silently widened. Serialising needs no such guard: Discrete.dtype exists
on every supported version and is int64 there, which is what was written
before.
test_space_serialize_deserialize compares the two serialized strings rather
than the spaces, which is why this went unnoticed.
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
_serialize_discretewrites"int64"for everyDiscretespace and_deserialize_discreteignores the field, so a space built with another dtypecomes back as a different space:
The comment on the line —
# this seems to be hardcoded in Gymnasium— wastrue when it was written.
Discretegained adtypeargument in Gymnasium1.3.0, and
DiscreteArray-style specs that default to int32 are common enoughthat a dataset can carry one.
test_space_serialize_deserializecompares the two serialized strings ratherthan the two spaces, so it stays green either way. That is why this went
unnoticed.
Changes
Serialising writes
str(space.dtype). No version guard is needed there:Discrete.dtypeexists on every supported Gymnasium and is int64 on theolder ones, which is exactly what was written before, so old and new files
agree.
Deserialising passes the dtype from Gymnasium 1.3.0 onwards.
pyproject.tomldeclares
gymnasium>=0.28.1, so on an older install a dataset that asks fora narrower dtype is reported:
Silently widening it would hand back a space the dataset never described.
Every file written before this change says int64, which matches the default,
so nothing existing hits that path.
The version check is the same shape as the one in
Farama-Foundation/Shimmy#157: major and minor compared as integers rather than
as strings, so 1.10 stays above 1.3, and no dependency on
packaging, whichis not in this project's requirements.
Tests
test_discrete_keeps_its_dtype, parametrised over int32 and int64. The int32case fails on
mainand passes here; int64 passes either way, which is wherethe two defaults coincide. The case is skipped with a reason below Gymnasium
1.3.0.
tests/test_serialization.pyis 21 passed,pre-commit runclean onboth files.
#345 touches the same two files, so whichever of the two lands second will need
a rebase — say the word and I will push it.