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.
meiertgrootes
left a comment
meiertgrootes
left a comment
There was a problem hiding this comment.
A very nice implementation. Just some minor comments and a suggestion on an elaborated inline documentation.
| return compute_masked_loss(pred, batch["monthly_patch"], batch["land_mask_patch"]) | ||
|
|
||
|
|
||
| def _compute_stats(dataset: Dataset): |
There was a problem hiding this comment.
I'm ok with this, but why are we dropping this at this iteration?
There was a problem hiding this comment.
The reason is the second point in this comment about bias/scale, with removing the initializer, we don't need this function anymore.
| 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), # (Month, lat, lon) |
There was a problem hiding this comment.
Here and in the following the implementation itself is fine. However, it is easily confusing for a non-initiated user. We refer to the basic time unit as a month, but in the dataset itself the basic time unit is either a day or an hour. This is only resolved by restructuring the dataset.
Wit that being the case, at this point it is also certainly conceiveable for the user that a non-integer (and even <1) value for the patch size in time would be permissible. This is clarified further below, but can still be significantly confusing.
I would suggest prefacing this section (or the dataset.py module as a whole) with a brief overview description of the approach adopted, i.e. that dataset.py reformats to a month dimension, that this must be integer and >0 and that the time dimension as present in the data is then folded in to monthly blocks.
There was a problem hiding this comment.
good point! I 'll add documentation.
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.