Address diffs v2.12.1 to v3 #907

chengzhuzhang · 2024-12-11T21:08:35Z

Description

Closes [Bug]: Investigate differences in plots produced with zppy test using Xarray/xCDAT codebase #906
Closes [Bug]: CDAT migration: potential performance or memory bottlenecks handling time-series data files #892
Related to EAMxx variables #880 (comment)

Regression test with v3 data

Related to #907

Add complete run script with v3 data
Run the complete run script using v2.12.1 and main
Perform regression test between v2.12.1 and main -- IN PROGRESS
- Variables with issues noted in this comment below

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)

This reverts commit a84b860.

tomvothecoder · 2024-12-11T23:31:07Z

Regression testing performed in #903, related to comment.

Copying here:

Here is the regression notebook that compares latest main vs. v2.12.1.

Matching number of .nc files produced (590)
501/590 are matching
4/590 are mismatching (nan positions)
85/590 are not equal

For the 85/590 are not equal to rtol=1e-4. These variables will need further investigation.

['/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/COREv2_Flux/COREv2_Flux-PminusE-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/CRU_IPCC/CRU-TREFHT-ANN-land_60S90N_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/CRU_IPCC/CRU-TREFHT-ANN-land_60S90N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/Cloud MISR/MISRCOSP-CLDTOT_TAU1.3_9.4_MISR-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/Cloud MISR/MISRCOSP-CLDTOT_TAU1.3_MISR-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/ERA5/ERA5-NET_FLUX_SRF-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/ERA5/ERA5-OMEGA-200-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/ERA5/ERA5-OMEGA-500-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/ERA5/ERA5-OMEGA-850-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/ERA5/ERA5-TREFHT-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/ERA5/ERA5-TREFHT-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/ERA5/ERA5-TREFHT-ANN-land_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/ERA5/ERA5-U-850-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/GPCP_OAFLux/GPCP_OAFLux-PminusE-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-NET_FLUX_SRF-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-OMEGA-200-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-OMEGA-500-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-OMEGA-850-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-TREFHT-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-TREFHT-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-TREFHT-ANN-land_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-TREFMNAV-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-TREFMNAV-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-TREFMXAV-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/MERRA2/MERRA2-TREFMXAV-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/SST_CL_HadISST/HadISST_CL-SST-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/SST_HadISST/HadISST-SST-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/SST_PD_HadISST/HadISST_PD-SST-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/lat_lon/SST_PI_HadISST/HadISST_PI-SST-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/meridional_mean_2d/MERRA2/MERRA2-OMEGA-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/CRU_IPCC/CRU-TREFHT-ANN-polar_N_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/CRU_IPCC/CRU-TREFHT-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/ERA5/ERA5-TREFHT-ANN-polar_N_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/ERA5/ERA5-TREFHT-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/ERA5/ERA5-TREFHT-ANN-polar_S_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/ERA5/ERA5-TREFHT-ANN-polar_S_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/ERA5/ERA5-U-850-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/ERA5/ERA5-U-850-ANN-polar_S_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFHT-ANN-polar_N_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFHT-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFHT-ANN-polar_S_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFHT-ANN-polar_S_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFMNAV-ANN-polar_N_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFMNAV-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFMNAV-ANN-polar_S_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFMNAV-ANN-polar_S_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFMXAV-ANN-polar_N_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFMXAV-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFMXAV-ANN-polar_S_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/MERRA2/MERRA2-TREFMXAV-ANN-polar_S_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/SST_CL_HadISST/HadISST_CL-SST-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/SST_CL_HadISST/HadISST_CL-SST-ANN-polar_S_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/SST_PD_HadISST/HadISST_PD-SST-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/SST_PD_HadISST/HadISST_PD-SST-ANN-polar_S_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/SST_PI_HadISST/HadISST_PI-SST-ANN-polar_N_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/polar/SST_PI_HadISST/HadISST_PI-SST-ANN-polar_S_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_2d/ERA5/ERA5-OMEGA-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_2d/MERRA2/MERRA2-OMEGA-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_2d_stratosphere/ERA5/ERA5-H2OLNZ-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_2d_stratosphere/ERA5/ERA5-H2OLNZ-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_2d_stratosphere/MERRA2/MERRA2-H2OLNZ-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_2d_stratosphere/MERRA2/MERRA2-H2OLNZ-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/ERA5/ERA5-TREFHT-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/ERA5/ERA5-TREFHT-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/MERRA2/MERRA2-TREFHT-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/MERRA2/MERRA2-TREFHT-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/MERRA2/MERRA2-TREFMNAV-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/MERRA2/MERRA2-TREFMNAV-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/MERRA2/MERRA2-TREFMXAV-ANN-global_ref.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/MERRA2/MERRA2-TREFMXAV-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/SST_CL_HadISST/HadISST_CL-SST-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/SST_PD_HadISST/HadISST_PD-SST-ANN-global_test.nc',
 '/global/cfs/cdirs/e3sm/www/e3sm_diags/complete_run/24-12-09-main/zonal_mean_xy/SST_PI_HadISST/HadISST_PI-SST-ANN-global_test.nc']

tomvothecoder · 2024-12-11T23:37:06Z

I will pull my regression testing notebooks over from PR #903 to this PR and work on debugging in this branch with you.

That way #903 can be dedicated to adapting the regression tests.

chengzhuzhang · 2024-12-12T17:21:09Z

thank you @tomvothecoder for summarizing the cases to investigate. It look like I'm running into environment issue while debugging with vscode #906 (comment). Let me know if the env works for you when debugging.

tomvothecoder · 2024-12-12T20:59:18Z

thank you @tomvothecoder for summarizing the cases to investigate. It look like I'm running into environment issue while debugging with vscode #906 (comment). Let me know if the env works for you when debugging.

I just replied here. The dev env works fine for me.

tomvothecoder · 2024-12-12T21:31:04Z

I think I found the root cause of the large diffs here and in your comment here, specifically with TREFHT and ANN.

`v2.12.1` code uses `genutil.udunits`

In v2.12.1, the convert_units() function for derived vars includes a portion of logic that calls genutil.udunits:

e3sm_diags/e3sm_diags/derivations/acme.py

Lines 93 to 97 in ca41b0e

    
           temp = udunits(1.0, var.units) 
        
           coeff, offset = temp.how(target_units) 
        
           var = coeff * var + offset 
        
           var.units = target_units

Latest `main` uses `cf_units.Unit` with new implementation logic

I replaced genutil.udnits with cf_units.Unit. It looks like the conversion is incorrect for TREFHT between K to C.

e3sm_diags/e3sm_diags/derivations/utils.py

Lines 85 to 93 in df10271

    
           temp = cf_units.Unit(var.attrs["units"]) 
        
           target = cf_units.Unit(target_units) 
        
           coeff, offset = temp.convert(1, target), temp.convert(0, target) 
        
           # Keep all of the attributes except the units. 
        
           with xr.set_options(keep_attrs=True): 
        
               var = coeff * var + offset 
        
           var.attrs["units"] = target_units

The implementation might not be able to handle all variables and/or the logic is incorrect, which leads to the massive diff found in your comment here.

Next steps

Investigate the cf_units.Unit code block in convert_units(). Fix as needed.

tomvothecoder · 2024-12-12T22:16:27Z

Investigate the cf_units.Unit code block in convert_units(). Fix as needed.

I just pushed ee009c1 (#907) to address this issue.

Next steps

Add this fix on PR EAMxx variables #880 and re-run plot for TREFHT-ANN-land to compare results
Perform another regression test with the complete run to compare against v2.12.1.

- Addresses performance bottleneck associated with attempting to load large datasets into memory. Time slicing reduces the size before loading into memory

chengzhuzhang added 3 commits December 11, 2024 13:03

address_diffs_v2.12.1_to_v3

a84b860

Revert "address_diffs_v2.12.1_to_v3"

796ca4c

This reverts commit a84b860.

add_test_scripts

0591c39

chengzhuzhang mentioned this pull request Dec 11, 2024

[Bug]: Investigate differences in plots produced with zppy test using Xarray/xCDAT codebase #906

Open

tomvothecoder mentioned this pull request Dec 11, 2024

[Feature]: Adapt CDAT Migration regression testing scripts for general purpose use #903

Draft

10 tasks

Fix incorrect logic for udunits conversions in convert_units()

ee009c1

tomvothecoder mentioned this pull request Dec 12, 2024

EAMxx variables #880

Open

9 tasks

tomvothecoder added 4 commits December 16, 2024 12:09

Add complete run script

528bac6

Update run script to save netCDF

8982c31

Update dev dir for regression notebook

09e853c

Add time slicing before .load() for performance

b9da444

- Addresses performance bottleneck associated with attempting to load large datasets into memory. Time slicing reduces the size before loading into memory

tomvothecoder force-pushed the address_diffs_v2.12.1_to_v3 branch from 46e77ef to b9da444 Compare December 17, 2024 19:12

This was referenced Dec 23, 2024

Enable CDAT-migrated E3SM Diags E3SM-Project/zppy#651

Open

Add support for multiple time series datasets via glob and fix enso_diags set #866

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address diffs v2.12.1 to v3 #907

Address diffs v2.12.1 to v3 #907

chengzhuzhang commented Dec 11, 2024 •

edited by tomvothecoder

Loading

tomvothecoder commented Dec 11, 2024 •

edited

Loading

tomvothecoder commented Dec 11, 2024

chengzhuzhang commented Dec 12, 2024

tomvothecoder commented Dec 12, 2024

tomvothecoder commented Dec 12, 2024

tomvothecoder commented Dec 12, 2024 •

edited

Loading

Address diffs v2.12.1 to v3 #907

Are you sure you want to change the base?

Address diffs v2.12.1 to v3 #907

Conversation

chengzhuzhang commented Dec 11, 2024 • edited by tomvothecoder Loading

Description

Regression test with v3 data

Checklist

tomvothecoder commented Dec 11, 2024 • edited Loading

tomvothecoder commented Dec 11, 2024

chengzhuzhang commented Dec 12, 2024

tomvothecoder commented Dec 12, 2024

tomvothecoder commented Dec 12, 2024

v2.12.1 code uses genutil.udunits

Latest main uses cf_units.Unit with new implementation logic

Next steps

tomvothecoder commented Dec 12, 2024 • edited Loading

Next steps

chengzhuzhang commented Dec 11, 2024 •

edited by tomvothecoder

Loading

tomvothecoder commented Dec 11, 2024 •

edited

Loading

`v2.12.1` code uses `genutil.udunits`

Latest `main` uses `cf_units.Unit` with new implementation logic

tomvothecoder commented Dec 12, 2024 •

edited

Loading