Data time subsetting#64
Conversation
|
See comment in PR #54 on replacing month positional encoding with a month-of-year feature in cyclical time embedding |
|
@meiertgrootes and @rogerkuou Hi, this PR is ready for review. Thanks! After merging #63 , I can update the notebook in this PR. |
rogerkuou
left a comment
rogerkuou
left a comment
There was a problem hiding this comment.
Hi @SarahAlidoost, thanks for the nice implementation.
I just have two small comments on an inline doc, and use of pytest fixture. Please feel free to merge after fixing them.
| time_dim: str = "time", | ||
| spatial_dims: Tuple[str, str] = ("lat", "lon"), | ||
| patch_size: Tuple[int, int] = (16, 16), # (lat, lon) | ||
| patch_size: Tuple[int, int, int] = (1, 16, 16), # (M, lat, lon) |
There was a problem hiding this comment.
Does "M" mean "month" here? I think it would be nice to have more informative docs about the input dimensions here.
| @pytest.fixture | ||
| def create_dummy_batch(): |
There was a problem hiding this comment.
I think pytest fixture are supposed to be used as "variables" but not "functions"?
So maybe there you can directly name this fixture dummy_batch
| and validating model architecture without executing ops. | ||
|
|
||
| """ | ||
| batch = create_dummy_batch |
There was a problem hiding this comment.
And if you rename the create_dummy_batch to dummy_batch, you do not need to assign this again. Here you are not calling create_dummy_batch() (as a method), so also indicates create_dummy_batch is already a fixture variable.
3 participants
closes #62
closes #42
🔴 this branch should be merged after #63
In this PR:
So the model is already learning a seasonal climatology and modulation, not relying on a fixed per-month correction.