Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR, titled "Normalisation fix", addresses several issues related to data normalization and batch preparation across the project’s data loading, model training, evaluation, and configuration. Key changes include:
- Updates to the CSV-based dataset classes to enable consistent per-sample and global normalization.
- Modifications to the collate functions and DataLoader construction to ensure that batches are prepared correctly for multiple model types.
- Updates to model implementations (e.g. Autoformer, Transformer, TimesNet) and configuration files to include explicit sequence and prediction lengths.
Reviewed Changes
Copilot reviewed 27 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cap/data/data.py | Updated CSVSequenceDataset and FedformerSequenceDataset normalization and batching |
| cap/training/trainer.py | Unified batch preparation across models using prepare_batch; added slicing for fedformer/timesnet outputs |
| cap/training/evaluator.py | Revised evaluation function to accumulate losses based on sample count rather than batch count |
| cap/models/Autoformer.py | Adjusted the shape of the zeros tensor for trend decomposition and output slicing |
| Various config files | Updated dataset paths, normalization flags, and added seq_len/pred_len parameters |
Comments suppressed due to low confidence (2)
cap/models/Autoformer.py:115
- Verify that changing the zeros tensor’s shape to use a fixed dimension of 1 (instead of x_dec.shape[2]) correctly aligns with the expected output dimensions; a mismatch here could lead to subtle errors in the trend and seasonal component recombination.
zeros = torch.zeros([x_dec.shape[0], self.pred_len, 1]).to(x_dec.device)
cap/data/data.py:175
- Consider adding a comment to explain why the normalized output is converted to a list (using .tolist()) instead of remaining as a NumPy array or tensor, to clarify the design decision for downstream processing.
out = ((out_arr - self.y_mean) / self.y_std).tolist()
Updated hyperparameters
There was a problem hiding this comment.
Pull Request Overview
This PR refactors data loading to support CSV-based datasets with optional normalization, standardizes batch preparation across models, and introduces device fallback and sequence-length overrides for certain models.
- Add CSVSequenceDataset and updated
get_dataloadersto handle CSV inputs and normalization flags - Introduce
BaseTimeSeriesModeland implementprepare_batchin all time-series models - Unified training/evaluation loops with model-agnostic batch handling; add device fallback in
train_et_model.py
Reviewed Changes
Copilot reviewed 27 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| train_et_model.py | Fallback to CPU on missing CUDA and override seq/pred lengths for Fedformer/TimesNet |
| requirement.txt | Loosened numpy/scipy version constraints |
| experiment/main.py | Reformatted arg parsing and unified dataloader invocation |
| cap/utils/base.py | Added BaseTimeSeriesModel for shared batch logic |
| cap/training/trainer.py | Extended train_model to support all model types and unified batch processing |
| cap/training/evaluator.py | Unified evaluate_model with model-agnostic batch handling |
| cap/models/transformer.py | Added prepare_batch method |
| cap/models/lstm.py | Subclassed BaseTimeSeriesModel and added prepare_batch |
| cap/models/TimesNet.py | Implemented prepare_batch |
| cap/models/Informer.py | Subclassed and added prepare_batch |
| cap/models/FEDFormer.py | Added prepare_batch |
| cap/models/Autoformer.py | Subclassed and added prepare_batch, fixed output slicing |
| cap/data/data.py | Introduced CSVSequenceDataset, FedformerSequenceDataset, and revamped get_dataloaders |
| cap/configs/*.yaml | Updated sample configs for normalization and seq/pred lengths |
| cap/main.py | Added TimesNet support in CLI |
Comments suppressed due to low confidence (1)
requirement.txt:1
- [nitpick] Filename
requirement.txtis unconventional; consider renaming torequirements.txtto follow Python packaging conventions.
# Core dependencies
| def train_model(train_loader, valid_loader, input_dim, output_dim, seq_len, pred_len, | ||
| hidden_dim=128, num_layers=2, epochs=1, lr=0.01, patience=5, | ||
| device="cuda" if torch.cuda.is_available() else "cpu", model_type='lstm'): | ||
| def train_model( |
There was a problem hiding this comment.
The num_layers parameter is required but has no default, yet calls to train_model only pass hidden_dim by keyword. Consider providing a default for num_layers or making it a keyword-only argument to avoid missing-argument errors.
|
|
||
| ds = CSVSequenceDataset(path, | ||
| seq_len=seq_len, | ||
| pred_len=pred_len, | ||
| normalization=normalization) |
There was a problem hiding this comment.
The FedformerSequenceDataset instance is immediately overwritten by CSVSequenceDataset on the next line, so the specialized Fedformer loader never gets used. You can fix this by using elif or scoping the CSV loader under an else branch.
| ds = CSVSequenceDataset(path, | |
| seq_len=seq_len, | |
| pred_len=pred_len, | |
| normalization=normalization) | |
| elif model_type.lower() == 'csv': | |
| ds = CSVSequenceDataset(path, | |
| seq_len=seq_len, | |
| pred_len=pred_len, | |
| normalization=normalization) |
| 'e_layers': 1, | ||
| 'd_layers': 1, | ||
| 'dropout': 0.05, | ||
| 'dropout': 0.05, |
There was a problem hiding this comment.
[nitpick] Duplicate 'dropout' key in the kwargs dict; the second entry overrides the first. Removing one will reduce confusion.
| 'dropout': 0.05, |
No description provided.