Skip to content

ENH: EpisodeData as dataclass #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jun 12, 2023
Merged

ENH: EpisodeData as dataclass #88

merged 5 commits into from
Jun 12, 2023

Conversation

younik
Copy link
Member

@younik younik commented Jun 8, 2023 •
edited
Loading

Description

Make EpisodeData a dataclass instead of NamedTuple and implement __repr__ method as suggested in #79

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@younik younik requested a review from balisujohn June 8, 2023 16:03
balisujohn
balisujohn reviewed Jun 9, 2023
View reviewed changes
Copy link
Collaborator

@balisujohn balisujohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to add a test for this, but otherwise it looks good to me.

@balisujohn balisujohn mentioned this pull request Jun 9, 2023
Closed
@younik
Copy link
Member Author

younik commented Jun 11, 2023

I think it would be good to add a test for this, but otherwise it looks good to me.

I added a test for it, thanks!

For testing, I copied the list of spaces that you used in your PR and added them to a file test_common; if we merge this first, you should update your test_serialization to use it

@younik younik requested a review from balisujohn June 11, 2023 19:28
@balisujohn
Copy link
Collaborator

Thanks for the test. I like the shared dependency for the test spaces so we avoid repeating definitions, I'll copy this pattern for the Dummy space registration for the unflattened spaces PR.

For the new test, can you also add an assert to the test that checks to see that the repr matches the expected repr string for each space?

@younik
Copy link
Member Author

younik commented Jun 12, 2023

Thanks for the test. I like the shared dependency for the test spaces so we avoid repeating definitions, I'll copy this pattern for the Dummy space registration for the unflattened spaces PR.

For the new test, can you also add an assert to the test that checks to see that the repr matches the expected repr string for each space?

done

balisujohn
balisujohn approved these changes Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@balisujohn balisujohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :^)

@balisujohn balisujohn merged commit 7ca16b3 into Farama-Foundation:main Jun 12, 2023
younik added a commit to younik/Minari that referenced this pull request Mar 21, 2025
@younik
ENH: EpisodeData as dataclass (Farama-Foundation#88)
688a353 
* episodedata as dataclass

* add test

* fix pre-commit

* add __init__ to test

* enhance test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balisujohn balisujohn balisujohn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@younik @balisujohn