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: deprecate setattr #1085

Open
MischaPanch opened this issue Apr 3, 2024 · 1 comment
Open

Batch: deprecate setattr #1085

MischaPanch opened this issue Apr 3, 2024 · 1 comment
Labels
Batch and Buffer Improvements in internal data structures, temporary label breaking changes Changes in public interfaces. Includes small changes or changes in keys refactoring No change to functionality

Comments

@MischaPanch
Copy link
Collaborator

There is no need to ever do something like

b = Batch(...)
b.new_attr = [1, 2, 3]

Batch behaves very surprisingly when this is done: sequences are cast to arrays, dicts are cast to Batch, datatypes and even lengths of arrays may change.

This can be solved by finding where setattr is being used (e.g., by adding a print statement to the implementation and running some examples), add a new method a la add_sequence, or even not support adding sequences post-hoc at all, e.g., in favor of creating a new batch with a method like with_added_sequence.

@MischaPanch MischaPanch added refactoring No change to functionality breaking changes Changes in public interfaces. Includes small changes or changes in keys 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

maxhuettenrauch commented Apr 11, 2024

This is actually an integral part of, for example, process_fn where derived values such as n-step returns are computed and added to the current batch of data.

end_flag = buffer.done.copy()
end_flag[buffer.unfinished_index()] = True
target_q = _nstep_return(rew, end_flag, target_q, indices, gamma, n_step)
batch.returns = to_torch_as(target_q, target_q_torch)

I guess the same should happen to __setitem__? Same parsing of values is happening there. Actually it happens already in __init__...

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 breaking changes Changes in public interfaces. Includes small changes or changes in keys refactoring No change to functionality
Development

No branches or pull requests

2 participants