Fix geo embeding - #92
Conversation
|
@meiertgrootes and @rogerkuou This PR fixes #91. Regarding the model performance see #93. Please let me know if something isnot clear. |
rogerkuou
left a comment
rogerkuou
left a comment
There was a problem hiding this comment.
Hi @SarahAlidoost , I had a look at the changes and they all look good! I haven't executed notebooks and I am planning to do it when Levante is back online.
I just have one tiny comment on the notwbook. Feel free to ignore.
Another general comment is, is this PR also aim to rename the daily/hourly variables to input? If this is the case, there are still many instances in dataset.py and st_encoder_decode.py, like these lines, but it seems you also get this tracked in your thirs point in issue #93
| @@ -689,7 +691,7 @@ | |||
| "name": "python", | |||
| "nbconvert_exporter": "python", | |||
| "pygments_lexer": "ipython3", | |||
| "version": "3.14.6" | |||
| "version": "3.11.14" | |||
There was a problem hiding this comment.
Maybe this is a very minor pick, but these changes caused by nb executions seem to be inevitable. I am either file with dorpping them in this PR, or accept them to exists and allowing future changes caused by futural runs.
@rogerkuou Hi, Thanks!, you can run the daily example notebook locally, I made the spatial subset even smaller, so it takes a few seconds for training loop to complete. Can you please run the notebook and see whether results, and the workflow are all good?
Correct! in this PR I didn't want to rename daily/hourly. Renaming variables is mentioned in issue #67 which I think it is not the priority now. But the third point in issue #93 is about the type of input (i.e. SST or anomaly), whether daily or hourly. |
meiertgrootes
left a comment
meiertgrootes
left a comment
There was a problem hiding this comment.
Looks good to me. Also waiting on Levante to run, but the adaptations make sense. Removing the spatial transformer does potentially loose some spatial correlation information, however, as we are temporally aggregating beforehand anyway that is likely not so critical. And it does alleviate the crop size dependence.
One very minor point on the tuning configuration.
Go ahead and merge when you see fit.
| time_features=time_features, | ||
| land_mask=ray.get(data_config["land_mask_data"]), | ||
| patch_size=data_config["patch_size"], # based on the patch_size in model | ||
| crop_size=data_config["crop_size"], # based on the patch_size in model |
There was a problem hiding this comment.
Please ignore if I just overlooked it, but I can't find the adapted keywords for the data_config in tuning.py
Yes, that's correct. I think training on the whole globe should help the model learn the spatial correlations. Once it's trained, we can check this and maybe add a spatial mixing layer in the decoder if needed 🤔 I'll add it to issue #93 so we don't forget.
|
3 participants
closes #91
The main changes in this PR:
SpatialTransfoemrst_encoder_decoderis mainly improving model performance.Todo: