Repository navigation
Conversation
dario-coscia
left a comment
There was a problem hiding this comment.
Looks good, I'm only missing what control should be. Maybe we can add a comment on it in the doc.
- linting is failing
GiovanniCanali
left a comment
There was a problem hiding this comment.
It is unclear to me whether store_data is called automatically by the condition or needs to be explicitly invoked by the user. If the latter, I would prefer to avoid this approach, as it introduces a different workflow compared to other conditions, is counterintuitive, and provides no safeguard against forgetting to call store_data.
I like the idea of distinguishing between target data and conditional data. However, the current argument names are not sufficiently descriptive. In particular, they do not make it clear that these arguments refer to subsets of the data features. Could we rename them to make their purpose more explicit?
| @@ -1,5 +1,7 @@ | |||
| """Module for the Time-Series Condition class.""" | |||
|
|
|||
| from typing import ClassVar | |||
There was a problem hiding this comment.
Is it necessary? Feels like we can avoid this by removing type suggestions at line 372.
Description
Updates TimeSeriesCondition with state / control column selectors (integer indices or LabelTensor labels). At each autoregressive step the model receives [y_k, a_k], only the predicted state is compared with the target and fed back, and control columns are never predicted. Temporal window parameters are now passed only to store_data, are not stored on the condition or in batches, and the constructor keeps the raw input until store_data is called; tests cover selectors, validation, controlled rollouts, and the new construction/storage split.
This PR fixes #867
Checklist