refactor: add ForecastBatch contract#637
Conversation
|
@sadamov @leifdenby please have a look 🙏 thank you |
sadamov
left a comment
There was a problem hiding this comment.
Nice PR @kshirajahere, thanks! One ask before merge: please add boundary as a 4th field of ForecastBatch now, so this lands cleanly alongside #635 in v0.8.0 without a re-rebase of every consumer. Relabelled as enhancement, proposed for v0.8.0.
| init_states: torch.Tensor | ||
| target_states: torch.Tensor | ||
| forcing: torch.Tensor | ||
| target_times: torch.Tensor | ||
|
|
||
| def to(self, device: torch.device | str) -> "ForecastBatch": | ||
| """Move all tensor fields to the same device.""" | ||
| return ForecastBatch( | ||
| init_states=self.init_states.to(device), | ||
| target_states=self.target_states.to(device), | ||
| forcing=self.forcing.to(device), | ||
| target_times=self.target_times.to(device), | ||
| ) |
There was a problem hiding this comment.
Add boundary and propagate through .to():
| init_states: torch.Tensor | |
| target_states: torch.Tensor | |
| forcing: torch.Tensor | |
| target_times: torch.Tensor | |
| def to(self, device: torch.device | str) -> "ForecastBatch": | |
| """Move all tensor fields to the same device.""" | |
| return ForecastBatch( | |
| init_states=self.init_states.to(device), | |
| target_states=self.target_states.to(device), | |
| forcing=self.forcing.to(device), | |
| target_times=self.target_times.to(device), | |
| ) | |
| init_states: torch.Tensor | |
| target_states: torch.Tensor | |
| forcing: torch.Tensor | |
| boundary: torch.Tensor | |
| target_times: torch.Tensor | |
| def to(self, device: torch.device | str) -> "ForecastBatch": | |
| """Move all tensor fields to the same device.""" | |
| return ForecastBatch( | |
| init_states=self.init_states.to(device), | |
| target_states=self.target_states.to(device), | |
| forcing=self.forcing.to(device), | |
| boundary=self.boundary.to(device), | |
| target_times=self.target_times.to(device), | |
| ) |
| return ForecastBatch( | ||
| init_states=batch[0], | ||
| target_states=batch[1], | ||
| forcing=batch[2], | ||
| target_times=batch[3], | ||
| ) |
There was a problem hiding this comment.
| return ForecastBatch( | |
| init_states=batch[0], | |
| target_states=batch[1], | |
| forcing=batch[2], | |
| target_times=batch[3], | |
| ) | |
| return ForecastBatch( | |
| init_states=batch[0], | |
| target_states=batch[1], | |
| forcing=batch[2], | |
| boundary=batch[3], | |
| target_times=batch[4], | |
| ) |
| return ForecastBatch( | ||
| init_states=init_states, | ||
| target_states=target_states, | ||
| forcing=forcing, | ||
| target_times=target_times, | ||
| ) |
There was a problem hiding this comment.
Pass boundary through here too (zero tensor of matching shape is fine until #635 wires up real loading).
| return prediction, batch.target_states, pred_std, batch.target_times | ||
|
|
||
| def training_step(self, batch): | ||
| batch = coerce_forecast_batch(batch) |
There was a problem hiding this comment.
Nit: training_step / validation_step / test_step each coerce, then common_step coerces again. Could lift to one place. Not blocking.
| - Add a named `ForecastBatch` contract for `WeatherDataset` batches and update | ||
| internal consumers to use field access [\#613](https://github.com/mllam/neural-lam/issues/613) @kshirajahere |
There was a problem hiding this comment.
Link the PR, not the issue.
| - Add a named `ForecastBatch` contract for `WeatherDataset` batches and update | |
| internal consumers to use field access [\#613](https://github.com/mllam/neural-lam/issues/613) @kshirajahere | |
| - Add a named `ForecastBatch` contract for `WeatherDataset` batches and update | |
| internal consumers to use field access [\#637](https://github.com/mllam/neural-lam/pull/637) @kshirajahere |
|
I really appreciate the effort to construct a data-structure for the entire training sample, rather than just a tuple of tensors, this will be fantastic 🌟 . But I think having a key in the named-tuple called What we probably need is either:
I would prefer 1. above, coercing multiple things into a single string is always brittle. In addition, I would actually advocate for the Thoughts? :) |
Refs #613
Describe your changes
Adds a named
ForecastBatchcontract forWeatherDatasetoutputs and migrates the current post-#208 internal consumers away from positional batch access.Changes
ForecastBatch(NamedTuple)inneural_lam/batch.py.ForecastBatchfromWeatherDataset.__getitem__.ForecasterModule,compute_standardization_stats.py, and tests to use named fields.default_collatesupport without a custom collate function.Issue Link
Closes #613
Type of change
Checklist before requesting a review
pullwith--rebaseoption if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee