Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize sst_trends #99

Merged
merged 8 commits into from
Oct 28, 2024
Merged

Generalize sst_trends #99

merged 8 commits into from
Oct 28, 2024

Conversation

uwagura
Copy link
Collaborator

@uwagura uwagura commented Oct 9, 2024

This PR adds the ability to run sst_trends.py using the config.yaml file. As always, figures of the new output are attached below, and I wouldn't read too much into any of actual metrics.

I added options to the process_glorys function in plot_common to work with script. You can now optionally specify a resample frequency that will be applied to the dataset before take the mean, as well as the whether you want to return the regrid object or the regridded glorys data. These options make the function compatible with this script, but I wasn't sure if the regrid option was good design, since it means that 'process_glorys' now technically has two different return values. I added type hints to reflect this, but I would love to hear people's opinions on this design and whether or not I should revert process_glorys to a single return type and avoid calling it in this script.

Similarly, I avoided calling process_oisst in this script because 1.) it regrids model output to the oisst data, while sst_trends regrids oisst data to the model output and 2.) it uses a conservative normed method to regrid, while sst_trends uses a bilinear method. It's probably possible to add options to process_oisst to handle these differences, but it would complicate the logic in process_oisst a bit more (especially if I added the two new options from process_glorys as well) , so I thought I'd open these changes to discussion as well before implementing them.

sst_trends_arctic
sst_trends_MHI
sst_trends_nep
sst_trends_nwa

@uwagura uwagura marked this pull request as ready for review October 10, 2024 16:26
@yichengt900
Copy link
Contributor

@uwagura Thanks for opening this PR! I’ll need to take a closer look later, but the code changes seem solid, and the results remain consistent. @andrew-c-ross, I would greatly appreciate your input, especially regarding the regridding aspect:

In 'sst_eval.py', we regrid the model to OISST and GLORYS to the model. Now, in 'sst_trend.py', we’re regridding both datasets to the model domain. I assume this is to ensure a fair comparison of temperature trends using the same domain. Do you think it would be better to just revert change made in this PR, or perhaps we could adjust the OISST regridding in sst_eval.py to maintain consistency across the scripts? Any feedback would be appreciated!

@andrew-c-ross
Copy link
Contributor

Re: regridding, my intention (which may not have been implemented in the original version of this script) was to conservatively interpolate the model data onto the observations in all cases where the observations have a resolution of 1/4 deg or coarser. For finer resolution products close to the same resolution as NWA12, I just bilinearly interpolated the observations onto the model grid. For very fine resolution products (only 4 km satellite chlorophyll), I did the same but with conservative interpolation. Thus, the intention was to interpolate the model to OISST, and interpolate GLORYS to the model. I doubt it makes much of a difference, but this was my plan to keep everything consistent.

@yichengt900
Copy link
Contributor

Hey @andrew-c-ross, thanks for the input! That clears things up for me. @uwagura, whenever you have a chance, could you update this PR to incorporate Andrew's comments?

@yichengt900 yichengt900 added CEFI_MOM6_RT_gaea_c6 bug Something isn't working and removed pass_CEFI_MOM6_RT bug Something isn't working labels Oct 16, 2024
@uwagura
Copy link
Collaborator Author

uwagura commented Oct 21, 2024

@yichengt900 , @andrew-c-ross , I have now updated the sst_trends script to regrid the model data to the oisst data. I had to make a couple of other changes to accommodate this, most notably
1.) oisst_delta is now defined as oisst_delta = mom_rg - oisst_trends
2.) The Model - Oisst plot now uses oisst_lonc and oisst_latc
3.) And I now use mom_rg and oisst_trends to calculate the skill scores instead of model_ave and oisst_rg.

Also, as a result of these changes, it looks like the domain of the Model - OISST plot is now slightly smaller, and the skill metrics are also slightly changed. Let me know if this seems reasonable:
sst_trends_nwa_mom_to_oisst

@uwagura
Copy link
Collaborator Author

uwagura commented Oct 21, 2024

Correction to my previous comment: it looks like I forgot to pass in the corner points to the regridder, so it was try to guess the corner points of the target grid, leading to the weird domain in my previous plot. This is fixed now, but the skill scores are still slightly different from the original plot.
sst_trends_nwa_mom_to_oisst_WITH_CORNERS
sst_trends_nep_mom_to_oisst_WITH_CORNERS
sst_trends_arctic_mom_to_oisst_WITH_CORNERS
sst_trends_MHI_mom_to_oisst_WITH_CORNERS

@uwagura uwagura changed the title Generalize sst Generalize sst_trends Oct 24, 2024
@uwagura uwagura requested a review from yichengt900 October 28, 2024 15:29
@yichengt900 yichengt900 merged commit 87b5c21 into main Oct 28, 2024
2 checks passed
@yichengt900 yichengt900 deleted the generalize_sst_trends branch October 30, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants