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

Batch: don't just strip off empty entries when creating batches #1089

Open
MischaPanch opened this issue Apr 3, 2024 · 5 comments
Open

Batch: don't just strip off empty entries when creating batches #1089

MischaPanch opened this issue Apr 3, 2024 · 5 comments
Labels
Batch and Buffer Improvements in internal data structures, temporary label bug Something isn't working

Comments

@MischaPanch
Copy link
Collaborator

MischaPanch commented Apr 3, 2024

Batch can sometimes just remove entries without ever telling the user:

Batch(info=[{"a": 1}, {}])
>>>
Batch(
    info: Batch(
              a: array([1]),
          ),
)

This doesn't happen always though:

Batch(a=[1, {}])
>>>
Batch(
    a: array([1, {}], dtype=object),
)

What's even worse, caused by #1088 , bogus values are created:

Batch(info=[{"a": 1}, {}, {"a": 2, "b": 1}])
>>> 
Batch(
    info: Batch(
              a: array([1, 2]),
              b: array([0, 1]),
          ),
)

Note that the middle entry was removed, and for the first entry 0 was added as the value of b.

This issue is strongly related to #1088 and #1087

@MischaPanch MischaPanch added bug Something isn't working Batch and Buffer Improvements in internal data structures, temporary label labels Apr 3, 2024
@MischaPanch MischaPanch added this to To do in Overall Tianshou Status via automation Apr 3, 2024
@maxhuettenrauch
Copy link
Collaborator

I'm quite sure these assignments shouldn't be valid for a batch, should they? If so, what should be the expected behavior in your given examples, especially if another batch item with the same key/structure should be concatenated or stacked? Maybe some kind of sanity check should run before constructing a batch, although this kind of data probably doesn't occur in the RL context.

@MischaPanch
Copy link
Collaborator Author

Yes, one way to solve this issue is to raise errors on invalid assignments. It's generally better to be strict. So many things can go wrong inside an RL training, we should help the users by eliminating as many possible sources of errors as we can

@MischaPanch
Copy link
Collaborator Author

MischaPanch commented Apr 16, 2024

We shouldn't completely eliminate object-type arrays though. For example our current vector envs return an array of dicts for the info entry. Custom envs are also free to return whatever they want as observations

@maxhuettenrauch
Copy link
Collaborator

Arrays of dicts should be fine (such as info entries), as long as the dict keys are consistent.

@MischaPanch
Copy link
Collaborator Author

I agree. It might not make sense to solve the batch related issues independently, maybe a single PR will have to address multiple of them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Batch and Buffer Improvements in internal data structures, temporary label bug Something isn't working
Development

No branches or pull requests

2 participants