Validate that metadata.time matches the batch size - #203
Validate that metadata.time matches the batch size#203priyDe (priyald) wants to merge 2 commits into
metadata.time matches the batch size#203Conversation
|
Wessel (@wesselb) tagging you as you offered on #188. Happy to change the approach if you'd rather keep |
|
@microsoft-github-policy-service agree |
`Metadata.time` documents one time per batch element, but nothing enforced it. The encoder adds the absolute time embedding as `(B, L, D) + (B, 1, D)`, so a mismatch silently broadcasts the batch dimension to `len(metadata.time)`. Add a `Batch.__post_init__` that checks the surface-level and atmospheric variables agree on the batch dimension and that `metadata.time` gives exactly one time per element, raising a clear error instead of failing later. `test_aurora_small` duplicated the data to batch size two without repeating `metadata.time`, so it relied on this broadcast. Repeat the time along with the data.
bd50773 to
a251842
Compare
The encoder adds the pressure encoding as `(B, C, L, D) + (1, len(atmos_levels), 1, D)`, which is the same broadcast that `metadata.time` was subject to. One level of data with four pressure levels returns four levels of prediction, three of them fabricated, and four levels of data with one pressure level silently discards three. Neither raises. Check it alongside the batch dimension, and factor the two now near-identical checks into `_consistent_size`. The level dimension is counted from the end because the decoder briefly constructs a batch before the history dimension is inserted, where `dim=2` is the height rather than the levels.
|
Pushed a second commit ( Looking for the same pattern elsewhere turned up one more instance: the pressure encoding in It seemed better to fix the same bug on both axes in one place than to open a near-identical second PR, but I am happy to split it if you would rather review them separately. |
|
Closing this in favour of #196, which fixes the same issue and was opened six weeks earlier. I should have found it before opening this one — it does not appear on the #188 thread, but that is my miss, not an excuse. Apologies for the extra tab, Wessel (@wesselb). For what it is worth, the two arrived at the same design independently, including using Two small gaps I ran into that #196 does not cover, if they are wanted once it lands:
Happy to open a small follow-up for those after #196 is merged, or to leave it be if you would rather keep the validation minimal. Either is fine by me. |
Fixes #188.
What's changed since the issue was filed
The issue reports silent corruption on 1.8.0: the output batch dimension is inflated to
len(metadata.time)with no error. On currentmainthe symptom is different —len(metadata.time) == 1still works, and anything larger now fails withfrom
lead_times[i]inPerceiver3DDecoder.forward. The root cause is unchanged: the encoderstill adds the absolute time embedding as
and that comment states an invariant which nothing enforces. So the failure has moved from
"quietly wrong" to "loud but misleading", and in the
len(metadata.time) < Bdirection it isstill quietly wrong.
The same bug on the pressure-level axis
While adding the check I looked for the same pattern elsewhere, and
Aurora3DEncoder.forwardhasit:
x_atmosis(B, C, L, D)and the encoding is(1, len(atmos_levels), 1, D), so a disagreementbetween
len(metadata.atmos_levels)and the level dimension of the atmospheric variablesbroadcasts exactly as the time embedding does. Running it on
AuroraSmall:mainatmos_levelsatmos_levelatmos_levelsRuntimeErrorfrom the addition (loud, so fine)The two silent cases are the same class of bug as #188, so I've handled both here rather than
opening a near-identical second PR. Happy to split it out if you'd prefer to review them
separately.
The fix
Batch.__post_init__now checks thatmetadata.timegives exactly one time per batch element,metadata.atmos_levelsgives exactly one level per level of those variables,raising a
ValueErrorat construction rather than deferring the failure. This matches thedocumented contract — "time: For every batch element, the time" — and is consistent with the
existing latitude and longitude validation in
Metadata.__post_init__. The two checks share asmall
_consistent_sizehelper so the logic exists once.One implementation note: the level dimension is counted from the end (
dim=-3).Perceiver3DDecoderconstructs a
Batchwhose atmospheric variables are(B, C, H, W), beforeAurora.forwardinsertsthe history dimension, so a positive index would read the height as a level count there.
One test needed changing, and it's worth flagging
test_aurora_smallduplicates the data to batch size two but leavesmetadata.timeat lengthone:
It passes on
mainbecause the broadcast happens to do the right thing in that direction, butit does contradict the documented contract. I've repeated the time along with the data. Happy to
take a different approach if you'd rather the
len(metadata.time) == 1case stay supported as anexplicit broadcast — that would be a small change to the check.
Tests
Added to
tests/test_batch.py:(B=1, times=2),(B=1, times=100),(B=2, times=1),(B=3, times=2);atmos_levelsis accepted, for 1, 2 and 4 levels;(C=1, levels=4),(C=4, levels=1),(C=4, levels=2),(C=2, levels=4);to,cropandregridstill produce valid batches.tests/test_batch.pyandtests/test_headers.pypass (73 tests). The full suite is117 passed, 1 failed, the failure being the pre-existingtest_aurora_smalldescribed below.ruff checkandruff format --checkare clean.Note on
test_aurora_smallIt fails for me at
0.04224444 / 276.22, marginally over the1e-4tolerance. This ispre-existing and unrelated: I get the identical value on unmodified
main, and it looks likethe machine-dependent numerics discussed in #169. Flagging it so it isn't mistaken for a
regression here.