Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CorrDiff: CWB Dataloader cleanups #593

Merged
merged 13 commits into from
Jul 26, 2024
Merged

CorrDiff: CWB Dataloader cleanups #593

merged 13 commits into from
Jul 26, 2024

Conversation

nbren12
Copy link
Collaborator

@nbren12 nbren12 commented Jul 15, 2024

Modulus Pull Request

Description

This PR deletes some unused code that made the CWB data loader harder to follow. It also uncomments, the corrdiff dataset tests.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The CHANGELOG.md is up to date with these changes.
  • An issue is linked to this pull request.

Dependencies

@nbren12
Copy link
Collaborator Author

nbren12 commented Jul 15, 2024

#589

@mnabian
Copy link
Collaborator

mnabian commented Jul 16, 2024

Need to merge this first for the CI to pass: #594

@mnabian mnabian requested a review from jleinonen July 16, 2024 00:50
@mnabian mnabian added the 4 - In Review Currently Under Review label Jul 16, 2024
@jleinonen
Copy link
Collaborator

Pretty big changes here, so can you check that you can start training ok, and that inference results with the pretrained checkpoint look ok?

@nbren12
Copy link
Collaborator Author

nbren12 commented Jul 16, 2024

Pretty big changes here, so can you check that you can start training ok, and that inference results with the pretrained checkpoint look ok?

Hey @Jussmith01. I'm not that spun-up on corrdiff training/inference, but did add a regression test against the real data, and ensured that first item in dataset was bit-for-bit identical throughout the refactor. Perhaps you can add a simple end-to-end test that runs the training and inference on a desktop in < 1 minute. Until such is in the code base, I think it's a bit laborious to make it a requirement for merging. Thoughts @mnabian?

@nbren12
Copy link
Collaborator Author

nbren12 commented Jul 16, 2024

Ready for re-review. I decided to leave the old reference configs intact since they have the hardcoded validation and other times. We'll probably want to include that in some form later on.

@jleinonen
Copy link
Collaborator

Pretty big changes here, so can you check that you can start training ok, and that inference results with the pretrained checkpoint look ok?

I'm not that spun-up on corrdiff training/inference, but did add a regression test against the real data, and ensured that first item in dataset was bit-for-bit identical throughout the refactor. Perhaps you can add a simple end-to-end test that runs the training and inference on a desktop in < 1 minute. Until such is in the code base, I think it's a bit laborious to make it a requirement for merging. Thoughts @mnabian?

Okay, I think that's enough for this PR. Maybe you could check that the normalize_input, normalize_output, denormalize_input, denormalize_output functions also work identically pre/post PR?

@nbren12
Copy link
Collaborator Author

nbren12 commented Jul 25, 2024

@jleinonen. normalize_input and normalize_output are covered by my existing regression check (since they are called in __getitem__). I will add a test for the denormalize methods.

@mnabian
Copy link
Collaborator

mnabian commented Jul 26, 2024

/blossom-ci

@mnabian mnabian merged commit 4525f08 into main Jul 26, 2024
1 check passed
@nbren12 nbren12 deleted the cleanup-normalizations branch July 26, 2024 16:23
mnabian added a commit that referenced this pull request Jul 29, 2024
mnabian added a commit that referenced this pull request Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review Currently Under Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants