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

Standardize regridding to conservative_normed #940

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

tomvothecoder
Copy link
Collaborator

@tomvothecoder tomvothecoder commented Feb 14, 2025

Description

  • Closes [Bug]: Investigate differences in xESMF and ESMF regridding that lead to "Model - Observation" plot diffs #938
  • e3sm_diags_driver/utils/regrid.py:
    • Update _align_grid_to_lower_res() to update "conservative" regrid method to "conservative_normed" for proper alignment with CDAT
    • Update _apply_land_sea_mask() to add a mask variable to the dataset for regridding, which helps prevent missing values from bleeding into the regridding.
    • Remove unused Notes comments referring to old CDAT-based functions that have been removed
  • e3sm_diags/driver/default_diags/lat_lon_model_vs_model.cfg and `e3sm_diags/driver/default_diags/lat_lon_model_vs_obs.cfg
    • Remove regrid_method = "bilinear" to default to "conservative" (which becomes "conservative_normed")

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have added tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

Comment on lines +376 to +384
# TODO: Remove this conditional once "conservative" references are updated
# to "conservative_normed" throughout the codebase.
# NOTE: this is equivalent to "conservative" in cdms2 ESMF. If
# "conservative" is chosen, it is updated to "conservative_normed". This
# logic can be removed once the CoreParameter.regrid_method default
# value is updated to "conservative_normed" and all sets have been
# refactored to use this function.
if method == "conservative":
method = "conservative_normed"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to #938 (comment)

I think I forgot to add this conditional in _align_grids_to_lower_res(), which resulted in the abnormal latitude edge in the polar plot here. This fixes that issue and hopefully more diffs between "Model - Observation" plots.

Copy link
Collaborator Author

@tomvothecoder tomvothecoder Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chengzhuzhang I just did a run with the specific variables from #930/#931 that have differences due to regridding. This change doesn't seem to affect the plot differences for these cases. The regression testing notebooks show the same differences between *_diff.nc files and the plot diffs look similar.

I think this is the new, expected result as far as I can tell (did not try the bounds dimension name alignment fix from #938).

Copy link
Collaborator Author

@tomvothecoder tomvothecoder Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix does improve the polar plots though, as you mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomvothecoder see comment here: #938 (comment). It looks like changing to convervative_normed has little impact, I'm interested to look at the plots and tables. Could you point me to the urls of both runs?

Copy link
Collaborator Author

@tomvothecoder tomvothecoder Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the links! I think we should use conservative_normed to replace bilinear. Now there is a caveat: here, look at the diff plot, there is a gap along the coastal line, in conservative_normed. And I realize that, part of the reason is that we are using a very coarse resolution file for land-sea mask. I'm creating a new land-sea mask to see if it helps.

@chengzhuzhang
Copy link
Contributor

chengzhuzhang commented Feb 19, 2025

@tomvothecoder Following:https://xesmf.readthedocs.io/en/latest/notebooks/Masking.html, I applied the fix (93bf5e3) by adding a mask variable to the dataset before regridding, it actually resolved the issue that mask bleeding into data:
After:
image

Before:
image

@chengzhuzhang
Copy link
Contributor

Maybe it is hopeful to wrap up this PR with some more testing?

@tomvothecoder
Copy link
Collaborator Author

Maybe it is hopeful to wrap up this PR with some more testing?

Here is the latest run with the mask fix: https://web.lcrc.anl.gov/public/e3sm/cdat-migration-fy24/25-02-19-branch-940-xesmf-diffs-mask-fix/viewer/index.html

@tomvothecoder tomvothecoder self-assigned this Feb 19, 2025
@tomvothecoder tomvothecoder changed the title Address "Model - Observation" plot differences due to xESMF Convert xESMF regridding from bilinear to conservative_normed Feb 19, 2025
@tomvothecoder tomvothecoder marked this pull request as ready for review February 19, 2025 21:22
@tomvothecoder tomvothecoder added bug Bug fix (will increment patch version) enhancement cdat-migration-fy24 CDAT Migration FY24 Task labels Feb 19, 2025
@tomvothecoder tomvothecoder changed the title Convert xESMF regridding from bilinear to conservative_normed Standardize regridding to conservative_normed Feb 19, 2025
@chengzhuzhang
Copy link
Contributor

@tomvothecoder thank you. I'm putting together all the urls and inspecting the metrics results change:
v2.12.1: https://web.lcrc.anl.gov/public/e3sm/cdat-migration-fy24/25-02-04-main-zppy-diffs/viewer/
Latest main with conservative: https://web.lcrc.anl.gov/public/e3sm/cdat-migration-fy24/25-02-04-branch-930-zppy-diffs/viewer/index.html
This branch with conservative_normed: https://web.lcrc.anl.gov/public/e3sm/cdat-migration-fy24/25-02-18-branch-940-xesmf-diffs/viewer/index.html
This branch with conservative_normed for all variables (no bilinear): https://web.lcrc.anl.gov/public/e3sm/cdat-migration-fy24/25-02-18-branch-940-xesmf-diffs-no-bilinear/viewer/index.html
Here is the latest run with the mask fix: https://web.lcrc.anl.gov/public/e3sm/cdat-migration-fy24/25-02-19-branch-940-xesmf-diffs-mask-fix/viewer/index.html

I noticed large difference in RMSE in SST plots to use conservative_normed compare to bilinear.
Conservative_normed:

Variables Unit Test_mean Ref._mean Mean_Bias Test_STD Ref._STD RMSE Correlation
SST global HadISST_CL degC 19.909 18.616 1.293 7.995 9.587 1.771 0.979
SST global HadISST_PD degC 19.909 18.73 1.178 7.995 9.589 1.839 0.977
SST global HadISST_PI degC 19.909 18.897 1.011 7.995 8.988 1.802 0.977

Variables Unit Test_mean Ref._mean Mean_Bias Test_STD Ref._STD RMSE Correlation
Bilinear:
SST global HadISST_CL degC 19.909 18.698 1.211 7.995 9.536 1.477 0.985
SST global HadISST_PD degC 19.909 18.807 1.102 7.995 9.543 1.551 0.984
SST global HadISST_PI degC 19.909 18.978 0.931 7.995 8.933 1.478 0.984

I'm going to plot out the regrided maps to examine and compare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fix (will increment patch version) cdat-migration-fy24 CDAT Migration FY24 Task enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Investigate differences in xESMF and ESMF regridding that lead to "Model - Observation" plot diffs
2 participants