You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Indexing Order: The State class currently supports batching along both Num Timesteps dimension(i.e. batch size (Timesteps, )) and along Num Timesteps, Num Trajectories dimension (i.e. batch size (Timesteps, Trajectory)). Flipping the indexing dimension (Timesteps, Trajectory) --> (Trajectory, Timesteps) maybe more user friendly to understand since conceptually Trajectories are a container over the State class.
The decision to keep the former might have been motivated by keeping an easy access to the sink/special states using the mask, but we can check if it's worth flipping to the latter. On first glance, it seems like lot of small changes need to be made to accommodate it.
The Trajectory class should be indifferent to the implementation of the State, Action class. Currently for eg: the __repr__() method uses the following hardcoded implementation assuming State always has a tensor attribute. This fails for eg on GraphStates implementation. This needs to be kept generic and derived from the states, actions, __repr__() method.
We can try to see if we can make the entry point for accessing States consistent. FlowMatching for example, directly accesses the States objects and other algorithms use Trajectory class to access the State attribute. We should check if we can have a general template for users implementing their own loss functions to keep things consistent. We can maybe have a simple example how to address both cases using the Trajectory class itself.
The text was updated successfully, but these errors were encountered:
Description
Indexing Order: The State class currently supports batching along both
Num Timesteps
dimension(i.e. batch size(Timesteps, )
) and alongNum Timesteps, Num Trajectories
dimension (i.e. batch size(Timesteps, Trajectory)
). Flipping the indexing dimension(Timesteps, Trajectory)
-->(Trajectory, Timesteps)
maybe more user friendly to understand since conceptually Trajectories are a container over the State class.The decision to keep the former might have been motivated by keeping an easy access to the sink/special states using the mask, but we can check if it's worth flipping to the latter. On first glance, it seems like lot of small changes need to be made to accommodate it.
The Trajectory class should be indifferent to the implementation of the State, Action class. Currently for eg: the
__repr__()
method uses the following hardcoded implementation assuming State always has a tensor attribute. This fails for eg on GraphStates implementation. This needs to be kept generic and derived from the states, actions,__repr__()
method.The text was updated successfully, but these errors were encountered: