[1849][model] Add optional latent Zarr writer - #1860
Conversation
…ams: ['..., latent']) Signed-off-by: evenmn <[email protected]>
|
@evenmn : can you please make sure the linter and unit tests pass. |
|
@grassesi : can you have a look at the changes in io.py. How far are you with refactoring the output writing? |
Signed-off-by: evenmn <[email protected]>
Signed-off-by: evenmn <[email protected]>
…Generator into feature/latent-zarr-writer
grassesi
left a comment
There was a problem hiding this comment.
Overall I like the idea of treating latent output just as another stream. One thing that needs to be addressed is that there is some masking logic that prevents the latent stream from trying to be processed during evaluation (esp. the to_xarray call will not work). Otherwise just some stylistic remarks.
I saw in the Issue that the latents should be eventually exposed via an JSON API? Would it make sense to already implement this and not try to piggyback on ZarrIO?
| # additionally yield latent output items if a latent stream name was provided | ||
| if self.latent_stream_name is not None and self.latents: | ||
| for s, fo_s in itertools.product(self.samples, self.forecast_steps): | ||
| key = ItemKey(int(s), int(fo_s), self.latent_stream_name) | ||
| latent_item = self._make_latent_item(key) | ||
| if latent_item is not None: | ||
| yield latent_item | ||
|
|
There was a problem hiding this comment.
Please try to wrap this logic into the above for iteration: having two yielding for loops works but is confusing at best. I see two alternatives:
- Mix the writing of latent items into the normal loop. Something like
for s, fo_s, fi_s in itertools.product(
self.samples, self.forecast_steps, self.streams.keys()
):
key = ItemKey(int(s), int(fo_s), fi_s)
if fi_s == LATENT_STREAM:
latent_item = self._make_latent_item(key)
if latent_item is not None:
yield latent_item
else:
yield self.extract(ItemKey(int(s), int(fo_s), fi_s))- have the writing of latent items in a separate method:
def latent_items(self):
if self.latents:
for s, fo_s in itertools.product(self.samples, self.forecast_steps):
key = ItemKey(int(s), int(fo_s), LATENT_STREAM)
latent_item = self._make_latent_item(key)
if latent_item is not None:
yield latent_item
...
with zarrio_writer(config.get_path_results(cf, mini_epoch)) as zio:
for subset in data.items():
zio.write_zarr(subset)
for latent in data.latent_items():
zio.write_zarr(latent)Option 2. is maybe a bit more clearer and more equivalent to the current solution and also provides more flexibility for the future. But Option 1 would be also fine with me.
| # optional name to use for latent pseudo-stream when yielding latent items | ||
| latent_stream_name: str | None = None |
There was a problem hiding this comment.
Please use a named constant for this.
Thanks for your feedback. Exposing the latent space via an JSON API is useful when running the model operationally. However, I still think we should export the latent state as a Zarr file, since this is useful for other applications, for instance explanable AI. |
Yes, json API is a separate issue and this PR should address writing the latent space to disc as zarr. |
clessig
left a comment
There was a problem hiding this comment.
Overall looks good. But validation_io will be refactored and it should be discussed how to best do the latent output going forward.
| output_streams = {} | ||
| for name in output_stream_names: | ||
| if name == "latent": | ||
| latent_stream_name = name |
There was a problem hiding this comment.
I don't understand the logic here. Wouldn't it be enough to have
if "latent" in output_stream_names
in l 158? Do we expect to have have multiple latent states?
| per_sample = {} | ||
| for lname, lval in latent_pred.items(): | ||
| if isinstance(lval, LatentState): | ||
| for field in ("z_pre_norm", "patch_tokens", "register_tokens", "class_token"): |
There was a problem hiding this comment.
The latent state that should be relevant for the output are the patch_tokens. These are used for the decoder. To be fully future proof we could have an argument which part of LatentState is written although it might be over-engineering.
|
|
||
| # collect latent outputs per forecast step and per sample (optional) | ||
| latents_all = [] | ||
| if latent_stream_name is not None: |
There was a problem hiding this comment.
This should go to a separate function.
Signed-off-by: evenmn <[email protected]>
Signed-off-by: evenmn <[email protected]>
| latent_stream_name: str | None = None | ||
| latent_stream_name: str | None = LATENT_STREAM |
There was a problem hiding this comment.
Please remove: since you are always using the default here anyway it makes no difference if self.latent_stream_name or LATENT_STREAM is used. But using using latent_stream_name clutters up the namespace/interface of OutputBatchData
There was a problem hiding this comment.
self.latent_stream_name is currently an alias to LATENT_STREAM which is never None. So please just use if self.latents (This should not disregard my previous comment on these lines.)
| stream_names = [stream.name for stream in cf.streams] | ||
| # include known pseudo-stream names (e.g. latent) so they are treated as known | ||
| if io.LATENT_STREAM not in stream_names: | ||
| stream_names.append(io.LATENT_STREAM) |
There was a problem hiding this comment.
Please remove and just use:
if io.LATENT_STREAM in output_stream_names:
output_streams[io.LATENT_STREAM] = Nonein ll. 136
| for name in output_stream_names: | ||
| if name == "latent": | ||
| if name == io.LATENT_STREAM: | ||
| latent_stream_name = name |
There was a problem hiding this comment.
Please remove this if clause, instead implement the suggestion I commented above
There was a problem hiding this comment.
Please use io.LATENT_STREAM here.
The choosen approach should translate relatively well into the refactored version. |
…get_latent_output' Signed-off-by: evenmn <[email protected]>
|
Thanks to both of you for the feedback, it truly improved this PR. I believe I have incorporated the suggested changes in my latest commit, but I still need to test the implementation before it gets merged in. |
Can you please test it as far as you can, and then I have a final look. |
Signed-off-by: evenmn <[email protected]>
@MatKbauer It comes with lat/lon, you can see the output dimensions in the first comment above. The time dimension is wrong (it has coordinate dimensions), but so is the time dimension in the ERA5 outputs |
…r-writer # Conflicts: # integration_tests/small1.yaml # integration_tests/small1_test.py # packages/common/src/weathergen/common/config.py # src/weathergen/train/trainer.py
There was a problem hiding this comment.
I successfully wrote some latent states to disk and appreciate they are contained seamlessly in the validation_chkpt00000_rank0000.zip.
Writing the latents with the 6x1h model resulted in some conflicts and I have tested and proposed a solution provided by Claude. There are some other small comments still, please have a look @evenmn.
With the new .zip instead of .zarr output, I'd suggest to update the PR description where the written latents can be plotted as follows:
store = ZipStore("validation_chkpt00000_rank0000.zip", mode="r")
z = zarr.open_group(store=store, zarr_format=3, mode="r")
That is, add the open_group explicitly to be compatible with the .zip format, which now is the default.
Also, please add a note to the PR description telling how latents can be written, i.e., when passing an according stream info to the inference command, such as
uv run inference ... --options test_config.output.streams=[ERA5,latent]
Edit: just realised that it is mentioned already in the description. Maybe make it more explicit with the uv run inference prefix.
@clessig, can you still have a look and merge if you agree?
…cition Signed-off-by: evenmn <[email protected]>
Removed group initialization in latent writer Co-authored-by: Matthias Karlbauer <[email protected]>
Move and clean up group attributes dict Co-authored-by: Matthias Karlbauer <[email protected]>
Remove duplicated lines Co-authored-by: Matthias Karlbauer <[email protected]>
Signed-off-by: evenmn <[email protected]>
…-writer # Conflicts: # src/weathergen/model/attention.py # src/weathergen/model/positional_encoding.py
… a stale devleop Signed-off-by: evenmn <[email protected]>
Signed-off-by: evenmn <[email protected]>
…sary files Signed-off-by: evenmn <[email protected]>
Signed-off-by: Even Marius Nordhagen <[email protected]>
| datasets.append(ds) | ||
|
|
||
| # TODO: missing forecast offset | ||
| return OutputItem(key=key, forecast_offset=None, latent=datasets) |
There was a problem hiding this comment.
Is there a reason for which we don't have forecast_offset included here?
There was a problem hiding this comment.
The idea here was that the latent-only items do not need the offset for target/prediction inclusion. If forecast_offset is 1 and forecast_step is 0, the offset_key method will return -1, which can cause strange behavior. I may be wrong
There was a problem hiding this comment.
I understand. The core question is: Do we want to write the initial condition t_0 as well when writing forecast latents, or do we start writing at t_0, which is in line with running a forecast after all.
There was a problem hiding this comment.
Yes, I think you're right. For a 40 step forecast the latent states are indexed 0 to 39, which might be confusing since index 0 is actually
There was a problem hiding this comment.
Good idea, to include the IC. This might mess up definitions at other places, though, as in forecast mode with offset: 1, we only write predictions and targets starting at
That is, when running in forecast mode, we want to have everything in step 0 to be empty. No predictions, targets, and latents.
If I'm not mistaken, we could realise this by changing this line in the latent writer from
now
for t_idx, latents_for_step in enumerate(data.latents):to
for t_idx, latents_for_step in enumerate(data.latents, start=data.forecast_offset):Signed-off-by: Even Marius Nordhagen <[email protected]>
Signed-off-by: Even Marius Nordhagen <[email protected]>
… latents_in_sample Signed-off-by: Even Marius Nordhagen <[email protected]>
…en and lantent_state_register_token -> register_token Signed-off-by: Even Marius Nordhagen <[email protected]>
Signed-off-by: evenmn <[email protected]>
Description
This PR introduces a writer for the latent vector. To avoid additional config options, I decided to add "latent" as a special case of a stream output, e.g:
uv run inference ... --options test_config.output.streams=[ERA5,latent]The latent output contains the latent vector itself, metadata to map healpix cells to coordinates, as well as extra features which may be useful for explainable AI applications. The combined ERA5 and latent output file takes the form (here for healpix level 5 and 2048 latent channels):
Going further, we can visualize the latent vector:
unzip validation_chkpt00000_rank0000.zip -d test.zarrHealpix level 4:

Healpix level 5:

Issue Number
Closes #1849
Is this PR a draft? Mark it as draft.
Checklist before asking for review
./scripts/actions.sh lint./scripts/actions.sh unit-test./scripts/actions.sh integration-testlaunch-slurm.py --time 60