Skip to content

Improve stdataset - #78

Open
SarahAlidoost wants to merge 38 commits into
mainfrom
improve_stdataset
Open

Improve stdataset#78
SarahAlidoost wants to merge 38 commits into
mainfrom
improve_stdataset

Conversation

@SarahAlidoost

@SarahAlidoost SarahAlidoost commented Jul 31, 2026

Copy link
Copy Markdown
Member

closes #75

In this PR:

  • data preparation is moved from STDataset class to util data preparation function
  • util data preparation function has an option to save preprocessed data to zarr --> but currently this is not used in our workflow
  • STDataset class has an option to keep data lazy in init and instead load it in getitem: save memory but slower --> data of training should not be lazy
  • loop over year is added to train and predict
  • data classes are created grouping input arguments
  • Scripts to run data preparation on HPC in scripts folder using dask distributed

I'm still testing the example_hourly notebook on HPC. but this PR can be reviewed.

TODO:

@SarahAlidoost
SarahAlidoost marked this pull request as ready for review July 31, 2026 13:46
@SarahAlidoost
SarahAlidoost requested review from meiertgrootes and rogerkuou and removed request for meiertgrootes and rogerkuou July 31, 2026 13:50
@SarahAlidoost
SarahAlidoost marked this pull request as draft July 31, 2026 14:04
@SarahAlidoost
SarahAlidoost marked this pull request as ready for review August 3, 2026 18:12
@SarahAlidoost
SarahAlidoost requested review from meiertgrootes and rogerkuou and removed request for rogerkuou August 3, 2026 18:12

@rogerkuou rogerkuou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SarahAlidoost , thanks for the reorganization! I have looked into the changes, and managed to run several epochs of the hourly notebook without OOM error.

The performance on my local for the hourly notebooks is ~7mins per epoch, with 4 threads computing and 2 workers reading. This is comparible with the timing profile in your notebook.

I only made one small comment about whether we should always loop in time. It is not critical. If your 1-year experiment on Levante works well, feel free to merge!

And PS. there is one linting issue raised by Ruff, I guess you also saw it.

Comment thread climanet/train.py Outdated

# Track unscaled loss for logging
epoch_loss += loss.detach()
for year in years:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we always want to loop by year, or for some circumstance we want to allow multiple years?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are training at a monthly aggregation anyway, so it doesn't really matter I think. The loop basically goes over all months, grouped by year (so allows for multiple years) within one epoch.

@rogerkuou rogerkuou mentioned this pull request Aug 4, 2026
Comment thread climanet/train.py
store_model: bool = True


def _move_batch_to_device(batch: dict, device: str):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a duplication of the class methos also used in predict.py. Given the very nice work you've done in cleaning and structuring one could consider moving it to utils and reusing in both modules. However, this is a most a suggestion - feel free to ignore

@meiertgrootes meiertgrootes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice and clean organization! One very minor comment and a response to @rogerkuou 's . Works locally. if/when levante test is complete feel free to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keep data lazy in class STDataset

3 participants