Skip to content
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.

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

ENH: improve MinariDataset #102

Merged
merged 9 commits into from
Jul 9, 2023

Conversation

younik
Copy link
Member

@younik younik commented Jun 28, 2023

Description

Add __getitem__and __len__() to MinariDataset and few refactors.

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 28, 2023 22:40
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.

Seems to run fine and produces a performant policy, I requested a few minor changes, and asked a few questions.

One more comment: It might be good to provide a requirements.txt for tutorials(though the version of Minari this will depend on isn't in a release yet, so I suppose it would have to be in a later PR)

docs/tutorials/q_learning_torch.py Outdated Show resolved Hide resolved
docs/tutorials/q_learning_torch.py Outdated Show resolved Hide resolved
docs/tutorials/q_learning_torch.py Outdated Show resolved Hide resolved
docs/tutorials/q_learning_torch.py Outdated Show resolved Hide resolved
docs/tutorials/q_learning_torch.py Outdated Show resolved Hide resolved
docs/tutorials/q_learning_torch.py Outdated Show resolved Hide resolved
@younik younik requested a review from balisujohn July 6, 2023 01:22
Copy link
Member

@rodrigodelazcano rodrigodelazcano left a comment

Choose a reason for hiding this comment

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

I just added some suggestions

docs/tutorials/behavioral_cloning.py Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
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.

Really minor change requests, and a reminder that we should add a version locked requirements.txt after the next minari release. Otherwise, looks good, and ready to merge, assuming 500 is a good amount of total reward for the final cloned policy.

docs/tutorials/behavioral_cloning.py Outdated Show resolved Hide resolved
docs/tutorials/behavioral_cloning.py Outdated Show resolved Hide resolved
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 :^)

@rodrigodelazcano
Copy link
Member

Looks good! Ready to merge

@rodrigodelazcano rodrigodelazcano merged commit ad1e6a6 into Farama-Foundation:main Jul 9, 2023
@younik younik deleted the torch-tutorial branch May 26, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants