Support independent input and output steps - #860
Conversation
alxmrs
left a comment
alxmrs
left a comment
There was a problem hiding this comment.
Initial review to agents.
| self._tensor_map = tensor_map | ||
| self.num_prognostic_channels = num_prognostic_channels | ||
| self.hist = hist | ||
| self.input_hist = hist |
There was a problem hiding this comment.
Maybe we can make this input_steps that is hist+1 to be consistent with the accounting of output_steps, rather than use output_hist which is a weird concept.
| ), | ||
| ) | ||
| input_steps: int | None = Field( | ||
| default=None, |
There was a problem hiding this comment.
This should be defaulted to 2 IMO, since his was defaulted to 1 before.
alxmrs
changed the title
|
@fomo-bot may I have your review? If you think the PR is ready for merge, will you approve it? |
|
Approved PR #860. I reviewed current head Validation:
Debug info
|
|
Hey @codex, will you review this PR? |
4ccafb7 to
cfbf15c
Compare
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex may I have your review on this PR? |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
cfbf15c to
2a54ac2
Compare
662a87f to
a66e9c6
Compare
amogh-gulati
left a comment
amogh-gulati
left a comment
There was a problem hiding this comment.
Looks good! just one small comment which came up
There was a problem hiding this comment.
🐑 : This is still old explanation right? where the inputs and outputs are coupled
There was a problem hiding this comment.
Good catch—the docstring was still describing only the coupled legacy behavior. I updated it to define windows in terms of independent input_steps and output_steps, including explicit 2-in/2-out and 2-in/1-out examples (2f49ac9).
3 participants
histhas been used to cover two concepts: the number of input steps and the number of output steps. These have always been decided together. This PR separates these two concepts so we can change them independently. Doing so, to me, clarifies what concept we actually need in various parts of howhistwas used -- code paths either need input or output steps, and this PR makes use of the proper concept by name now instead of implicitly. I find that this makes reading various data flow paths more understandable than before.🤖 below
Summary
data.output_stepsconfiguration, defaulting tohist + 1This enables
hist: 1, output_steps: 1: two input states predict one future state, and each autoregressive call shifts[x(t-1), x(t)]to[x(t), x_hat(t+1)].Existing configs omit
output_stepsand retain their currenthist + 1output blocks.Validation
uvx pre-commit run --all-files.venv/bin/pytest -m 'not manual and not cuda' -q