Validate the grid, the history dimension and the latitudes and longitudes - #204
Open
priyDe (priyald) wants to merge 1 commit into
Open
Validate the grid, the history dimension and the latitudes and longitudes#204priyDe (priyald) wants to merge 1 commit into
priyDe (priyald) wants to merge 1 commit into
Conversation
…udes `Batch` documents the shapes of its variables but does not enforce them. Add a `__post_init__` that checks the three invariants not covered by microsoft#196: * every surface-level, static and atmospheric variable describes the same grid; * the surface-level and atmospheric variables agree on the history dimension; * the latitudes and longitudes match that grid, in both the vector and the matrix form. The latitudes and longitudes are checked in the encoder today, but with a bare `assert` that carries no message and is removed under `python -O`. The other two surface as shape errors from inside torch, which do not say which variable is wrong. Raising at construction is consistent with the existing latitude and longitude validation in `Metadata.__post_init__`. The spatial shape is read from the last two dimensions because the static variables are expanded to `(b, t, h, w)` inside the model and the decoder builds a batch before the history dimension is inserted. The history dimension is checked only on variables that still have one, for the same reason. An empty `surf_vars` or `atmos_vars` is left alone rather than treated as an error, since running without atmospheric variables is suggested in microsoft#176.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #196, and should be merged after it.
#196 validates that
metadata.timeandmetadata.atmos_levelsmatch the data. This adds theremaining shape invariants that
Batchdocuments but does not enforce. There is no functionaloverlap: nothing here touches
metadata.timeoratmos_levels.What it checks
Why these three
The latitudes and longitudes are already checked in
Aurora3DEncoder.forward, but withwhich carries no message, fires deep inside the forward pass rather than where the batch was
built, and is removed entirely under
python -O. The other two currently surface as shape errorsfrom inside torch, for example
Sizes of tensors must match except in dimension 2, which does nottell the caller which variable is wrong.
Raising at construction, next to the existing latitude and longitude validation in
Metadata.__post_init__, points at the actual problem.Two implementation notes
The spatial shape is read from the last two dimensions rather than the full shape. The static
variables are
(h, w)at the public API butAurora.forwardexpands them to(b, t, h, w), andPerceiver3DDecoderconstructs aBatchbefore the history dimension is inserted. Comparing fullshapes would reject both.
For the same reason the history dimension is only checked on variables that still have one, that
is surface-level variables with four dimensions and atmospheric variables with five.
Empty variable dictionaries
An empty
surf_varsoratmos_varsis left alone rather than treated as an error, since runningwithout atmospheric variables is something you suggested trying in #176. There is a test for it.
Merge conflict, and how I would like to handle it
This creates
Batch.__post_init__, and so does #196. They will conflict textually even though thechecks are disjoint. #196 is approved and should land first. Once it does, say the word and I will
rebase this onto it so the two sets of checks sit in one method. Happy to do that at any point,
including before you review, if that is easier.
Tests
Added to
tests/test_batch.py:atmos_varsis accepted;to,cropandregridstill produce valid batches.Full suite passes apart from
test_aurora_small, which fails for me at0.04224444 / 276.22onunmodified
mainas well. That looks like the machine-dependent numerics in #169, so I am flaggingit rather than claiming it is related to this change.
ruff checkandruff format --checkareclean.