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

change create dataset from buffer to ensure same behavior with create data from env #146

Closed

Conversation

im-Kitsch
Copy link
Contributor

Description

Hi,

I think it would be better to keep create_from_buffer and create_from_collevtor_env having same bahavior. If we create dataset from collector env, the last episode will be marked as truncated if it's incomplete trajectory. But if create dataset from replay buffers, incomplete trajector will be rejected.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Screenshots

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

@balisujohn
Copy link
Collaborator

Thanks for this contribution, and I agree with the direction. It looks correct to me. Please write a test which makes sure a partial episode is correctly marked with a truncation when calling create_dataset_from_buffers , and I will merge this.

@im-Kitsch
Copy link
Contributor Author

im-Kitsch commented Sep 11, 2023

Hi @balisujohn ,
I added test, could you take a look of new commit?
Thanks

minari/utils.py Outdated Show resolved Hide resolved
@balisujohn
Copy link
Collaborator

Once typos I pointed out are fixed and pre-commit is passing, this is ready to merge :^)

@im-Kitsch
Copy link
Contributor Author

Hi, finally I turned back to fix the problem. Could you check it again?

Copy link
Member

@younik younik left a comment

Choose a reason for hiding this comment

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

@im-Kitsch thanks for doing this; it seems good. However, would you mind to wait until #133 is merged?
The problem will be solved by that PR, but we may want to add your test

Comment on lines +469 to +472
else:
assert (
eps_buff["terminations"][-1] or eps_buff["truncations"][-1]
), "Each episode must be terminated or truncated before adding it to a Minari dataset"
Copy link
Member

Choose a reason for hiding this comment

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

it looks like this branch is not very useful, I would remove it

@im-Kitsch
Copy link
Contributor Author

@im-Kitsch thanks for doing this; it seems good. However, would you mind to wait until #133 is merged? The problem will be solved by that PR, but we may want to add your test

@younik Hi, thanks for the reply, looks after #133 , merging data would diable copy choice, then this branch will not be very useful. Let's wait merging of #133

@younik
Copy link
Member

younik commented Oct 25, 2023

Right @im-Kitsch, thanks for your help :)

@younik younik closed this Oct 25, 2023
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.

3 participants