[Refactor]: Validate fix in PR #750 for #759 #815

tomvothecoder · 2024-05-14T18:25:20Z

Description

Closes [Refactor]: CDAT Migration: cdms2.open slice flag adds additional time coordinate which affects downstream time series operations and diffs with Xarray code #759

5/30/24

Re-run ex1.py and perform regression test again
Check for diffs
- NET_FLUX_SRF and RESTOM had the largest diffs with the slice flag -- now fixed
Debug diffs
- Fix "TREFHT" land large diff with mean metrics -- due to incorrect Xarray datasets being passed to _subset_on_region() (need to pass objects with land mask applied)
- Fix TREFHT land mean ref regridded data (11.59% diff) -- address this later in a separate GH issue ([Bug]: Figure out why TREFHT regridded reference variable is not regridding on main with ESMF bilinear #817)

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)

tomvothecoder

This PR validates the replication of the slice flag feature added in PR #750 to fix #759.

I re-ran ex1.py and found that all .nc files produced are now within the relative tolerance.

For metrics, all but one is within the relative tolerance. This single metric is related to the regridded reference data for "TREFHT" with the land region. In trefht.py, I compare the .nc file for the regridded reference data between xCDAT/xESMF and CDAT/ESMF. I found that CDAT/ESMF doesn't seem to actually regrid the data since the original and regridded reference variables are the same, as noted in this PR review comment. I think this is leading to the large diffs along the coast lines, as shown below.

xCDAT	CDAT	Diff

In PR #794, I noticed how bilinear regridding with xCDAT/xESMF produces nan location mismatches when comparing land sea masks to CDAT/ESMF. I think we should move forward for now and debug this more later on in a separate GitHub issue (#817).

tomvothecoder · 2024-06-03T20:50:00Z

auxiliary_tools/cdat_regression_testing/759-slice-flag/debug/trefht.py

+# %%
+"""
+1. Compare CDAT original data with regridded data
+
+Result: There is no difference between the original and regridded reference
+data.
+"""
+# Test closeness (same)
+np.testing.assert_allclose(mv2, mv2_reg, atol=0, rtol=0)
+
+mv2_filled = mv2.filled(np.nan)
+mv2_reg_filled = mv2_reg.filled(np.nan)
+
+#  Count of np.nan: 22170 vs. 22170 (same)
+np.count_nonzero(np.isnan(mv2_filled.flatten())) == np.count_nonzero(
+    np.isnan(mv2_reg_filled.flatten())
+)
+# Sum: -68837.01023016105 vs. -68837.01023016105 (same)
+np.nansum(mv2_filled) == np.nansum(mv2_reg_filled)
+# Mean: -6.342086809486 == -6.342086809486 (same)
+np.nanmean(mv2_filled) == np.nanmean(mv2_reg_filled)


I noticed that for the "TREFHT" variable, the CDAT code on main reaches the conditional to regrid the reference variable (mv2) to the test variable (mv1).

e3sm_diags/e3sm_diags/driver/utils/general.py

Lines 246 to 254 in c8404ab

# use nlat to decide data resolution, higher number means higher data

# resolution. For the difference plot, regrid toward lower resolution

if len(axes1[1]) <= len(axes2[1]):

mv_grid = mv1.getGrid()

mv1_reg = mv1

mv2_reg = mv2.regrid(

mv_grid, regridTool=regrid_tool, regridMethod=regrid_method

)

mv2_reg.units = mv2.units

However, the regridded reference data (mv2_reg) is the same as the original reference data (mv2) as shown in the comparison script above. No regridding seems to be happening. I opened #817 to address this later.

tomvothecoder · 2024-06-03T20:50:40Z

auxiliary_tools/cdat_regression_testing/759-slice-flag/debug/trefht.py

+"""
+2. Compare regridded data between xCDAT and CDAT
+
+Result: The regridded reference data produced by xCDAT results in more `np.nan`,
+which subsequently results in different np.nan locations, sum, and mean.
+
+In https://github.com/E3SM-Project/e3sm_diags/pull/794, I noted that there
+seems tobe a minor difference in how the bilinear regridding works with xCDAT + xESMF vs.
+CDAT + ESMF.
+"""
+# Test closeness (x and y nan location mismatch)
+np.testing.assert_allclose(ds_b_regrid[var_key].values, mv2_reg, atol=0, rtol=0)
+
+# Count of np.nan: 23150 vs. 22170 (different)
+np.count_nonzero(np.isnan(ds_b_regrid[var_key].values.flatten())) == np.count_nonzero(
+    np.isnan(mv2_reg_filled.flatten())
+)
+# Sum: -77674.06388742107 vs. -68837.01023016105 (different)
+np.nansum(ds_b_regrid[var_key].values) == np.nansum(mv2_reg_filled.data)
+# Mean: -7.866524598685545 vs. -6.342086809486 (different)
+np.nanmean(ds_b_regrid[var_key].values) == np.nanmean(mv2_reg_filled.data)
+
+# %%


I noted how xCDAT actually performs the regridding. In debug_TREFHT_diff.png we can see the difference in the plots.

tomvothecoder · 2024-06-03T21:08:37Z

e3sm_diags/driver/utils/regrid.py

    if "global" not in region:
-        ds_test_new = _subset_on_region(ds_test, var_key, region)
-        ds_ref_new = _subset_on_region(ds_ref, var_key, region)
+        ds_test_new = _subset_on_region(ds_test_new, var_key, region)
+        ds_ref_new = _subset_on_region(ds_ref_new, var_key, region)


Incorrect dataset objects were being passed to _subset_on_region. If land sea mask was applied, it was being ignored here and downstream.

This affected lat_lon and polar.

tomvothecoder · 2024-06-11T16:39:18Z

Hey @chengzhuzhang, just an FYI that I added functionality to replicate the cdms2 "ccb flag for non-submonthly data (#759).

I tested ex1.py which uses time series data and all of the results check out except one (new issue #817). You can refer to the above review comment for more info.

You can review the new code for this functionality here in dataset_xr.py: https://github.com/E3SM-Project/e3sm_diags/pull/750/files#diff-35006a05805f6f5876c9fe70b5281c865955b8a82d1574fd7f62d6efda623975

chengzhuzhang · 2024-06-13T19:44:46Z

@tomvothecoder great work to support the slice flag and match the results with main! Also thanks for careful documentation on the final straggler and I agree that we should address in a separate PR..

Refer to the PR for more information because the changelog is massive. Update build workflow to run on `cdat-migration-fy24` branch CDAT Migration Phase 2: Add CDAT regression test notebook template and fix GH Actions build (#743) - Add Makefile for quick access to multiple Python-based commands such as linting, testing, cleaning up cache and build files - Fix some lingering unit tests failure - Update `xcdat=0.6.0rc1` to `xcdat >=0.6.0` in `ci.yml`, `dev.yml` and `dev-nompi.yml` - Add `xskillscore` to `ci.yml` - Fix `pre-commit` issues CDAT Migration Phase 2: Regression testing for `lat_lon`, `lat_lon_land`, and `lat_lon_river` (#744) - Add Makefile that simplifies common development commands (building and installing, testing, etc.) - Write unit tests to cover all new code for utility functions - `dataset_xr.py`, `metrics.py`, `climo_xr.py`, `io.py`, `regrid.py` - Metrics comparison for `cdat-migration-fy24` `lat_lon` and `main` branch of `lat_lon` -- `NET_FLUX_SRF` and `RESTOM` have the highest spatial average diffs - Test run with 3D variables (`_run_3d_diags()`) - Fix Python 3.9 bug with using pipe command to represent Union -- doesn't work with `from __future__ import annotations` still - Fix subsetting syntax bug using ilev - Fix regridding bug where a single plev is passed and xCDAT does not allow generating bounds for coordinates of len <= 1 -- add conditional that just ignores adding new bounds for regridded output datasets, fix related tests - Fix accidentally calling save plots and metrics twice in `_get_metrics_by_region()` - Fix failing integration tests pass in CI/CD - Refactor `test_diags.py` -- replace unittest with pytest - Refactor `test_all_sets.py` -- replace unittest with pytest - Test climatology datasets -- tested with 3d variables using `test_all_sets.py` CDAT Migration Phase 2: Refactor utilities and CoreParameter methods for reusability across diagnostic sets (#746) - Move driver type annotations to `type_annotations.py` - Move `lat_lon_driver._save_data_metrics_and_plots()` to `io.py` - Update `_save_data_metrics_and_plots` args to accept `plot_func` callable - Update `metrics.spatial_avg` to return an optionally `xr.DataArray` with `as_list=False` - Move `parameter` arg to the top in `lat_lon_plot.plot` - Move `_set_param_output_attrs` and `_set_name_yr_attrs` from `lat_lon_driver` to `CoreParameter` class Regression testing for lat_lon variables `NET_FLUX_SRF` and `RESTOM` (#754) Update regression test notebook to show validation of all vars Add `subset_and_align_datasets()` to regrid.py (#776) Add template run scripts CDAT Migration Phase: Refactor `cosp_histogram` set (#748) - Refactor `cosp_histogram_driver.py` and `cosp_histogram_plot.py` - `formulas_cosp.py` (new file) - Includes refactored, Xarray-based `cosp_histogram_standard()` and `cosp_bin_sum()` functions - I wrote a lot of new code in `formulas_cosp.py` to clean up `derivations.py` and the old equivalent functions in `utils.py` - `derivations.py` - Cleaned up portions of `DERIVED_VARIABLES` dictionary - Removed unnecessary `OrderedDict` usage for `cosp_histogram` related variables (we should do this for the rest of the variables in in #716) - Remove unnecessary `convert_units()` function calls - Move cloud levels passed to derived variable formulas to `formulas_cosp.CLOUD_BIN_SUM_MAP` - `utils.py` - Delete deprecated, CDAT-based `cosp_histogram` functions - `dataset_xr.py` - Add `dataset_xr.Dataset._open_climo_dataset()` method with a catch for dataset quality issues where "time" is a scalar variable that does not match the "time" dimension array length, drops this variable and replaces it with the correct coordinate - Update `_get_dataset_with_derivation_func()` to handle derivation functions that require the `xr.Dataset` and `target_var_key` args (e.g., `cosp_histogram_standardize()` and `cosp_bin_sum()`) - `io.py` - Update `_write_vars_to_netcdf()` to write test, ref, and diff variables to individual netCDF (required for easy comparison to CDAT-based code that does the same thing) - Add `cdat_migration_regression_test_netcdf.ipynb` validation notebook template for comparing `.nc` files CDAT Migration Phase 2: Refactor `zonal_mean_2d()` and `zonal_mean_2d_stratosphere()` sets (#774) Refactor 654 zonal mean xy (#752) Co-authored-by: Tom Vo <[email protected]> CDAT Migration - Update run script output directory to NERSC public webserver (#793) [PR]: CDAT Migration: Refactor `aerosol_aeronet` set (#788) CDAT Migration: Test `lat_lon` set with run script and debug any issues (#794) CDAT Migration: Refactor `polar` set (#749) Co-authored-by: Tom Vo <[email protected]> Align order of calls to `_set_param_output_attrs` CDAT Migration: Refactor `meridional_mean_2d` set (#795) CDAT Migration: Refactor `aerosol_budget` (#800) Add `acme.py` changes from PR #712 (#814) * Add `acme.py` changes from PR #712 * Replace unnecessary lambda call Refactor area_mean_time_series and add ccb slice flag feature (#750) Co-authored-by: Tom Vo <[email protected]> [Refactor]: Validate fix in PR #750 for #759 (#815) CDAT Migration Phase 2: Refactor `diurnal_cycle` set (#819) CDAT Migration: Refactor annual_cycle_zonal_mean set (#798) * Refactor `annual_cycle_zonal_mean` set * Address PR review comments * Add lat lon regression testing * Add debugging scripts * Update `_open_climo_dataset()` to decode times as workaround to misaligned time coords - Update `annual_cycle_zonal_mean_plot.py` to convert time coordinates to month integers * Fix unit tests * Remove old plotter * Add script to debug decode_times=True and ncclimo file * Update plotter time values to month integers * Fix slow `.load()` and multiprocessing issue - Due to incorrectly updating `keep_bnds` logic - Add `_encode_time_coords()` to workaround cftime issue `ValueError: "months since" units only allowed for "360_day" calendar` * Update `_encode_time_coords()` docstring * Add AODVIS debug script * update AODVIS obs datasets; regression test results --------- Co-authored-by: Tom Vo <[email protected]> CDAT Migration Phase 2: Refactor `qbo` set (#826) CDAT Migration Phase 2: Refactor tc_analysis set (#829) * start tc_analysis_refactor * update driver * update plotting * Clean up plotter - Remove unused variables - Make `plot_info` a constant called `PLOT_INFO`, which is now a dict of dicts - Reorder functions for top-down readability * Remove unused notebook --------- Co-authored-by: tomvothecoder <[email protected]> CDAT Migration Phase 2: Refactor `enso_diags` set (#832) CDAT Migration Phase 2: Refactor `streamflow` set (#837) [Bug]: CDAT Migration Phase 2: enso_diags plot fixes (#841) [Refactor]: CDAT Migration Phase 3: testing and documentation update (#846) CDAT Migration Phase 3 - Port QBO Wavelet feature to Xarray/xCDAT codebase (#860) CDAT Migration Phase 2: Refactor arm_diags set (#842) Add performance benchmark material (#864) Add function to add CF axis attr to Z axis if missing for downstream xCDAT operations (#865) CDAT Migration Phase 3: Add Convective Precipitation Fraction in lat-lon (#875) CDAT Migration Phase 3: Fix LHFLX name and add catch for non-existent or empty TE stitch file (#876) Add support for time series datasets via glob and fix `enso_diags` set (#866) Add fix for checking `is_time_series()` property based on `data_type` attr (#881) CDAT migration: Fix African easterly wave density plots in TC analysis and convert H20LNZ units to ppm/volume (#882) CDAT Migration: Update `mp_partition_driver.py` to use Dataset from `dataset_xr.py` (#883) CDAT Migration - Port JJB tropical subseasonal diags to Xarray/xCDAT (#887) CDAT Migration: Prepare branch for merge to `main` (#885) [Refactor]: CDAT Migration - Update dependencies and remove Dataset._add_cf_attrs_to_z_axes() (#891) CDAT Migration Phase 2: Refactor core utilities and `lat_lon` set (#677) Refer to the PR for more information because the changelog is massive. Update build workflow to run on `cdat-migration-fy24` branch CDAT Migration Phase 2: Add CDAT regression test notebook template and fix GH Actions build (#743) - Add Makefile for quick access to multiple Python-based commands such as linting, testing, cleaning up cache and build files - Fix some lingering unit tests failure - Update `xcdat=0.6.0rc1` to `xcdat >=0.6.0` in `ci.yml`, `dev.yml` and `dev-nompi.yml` - Add `xskillscore` to `ci.yml` - Fix `pre-commit` issues CDAT Migration Phase 2: Regression testing for `lat_lon`, `lat_lon_land`, and `lat_lon_river` (#744) - Add Makefile that simplifies common development commands (building and installing, testing, etc.) - Write unit tests to cover all new code for utility functions - `dataset_xr.py`, `metrics.py`, `climo_xr.py`, `io.py`, `regrid.py` - Metrics comparison for `cdat-migration-fy24` `lat_lon` and `main` branch of `lat_lon` -- `NET_FLUX_SRF` and `RESTOM` have the highest spatial average diffs - Test run with 3D variables (`_run_3d_diags()`) - Fix Python 3.9 bug with using pipe command to represent Union -- doesn't work with `from __future__ import annotations` still - Fix subsetting syntax bug using ilev - Fix regridding bug where a single plev is passed and xCDAT does not allow generating bounds for coordinates of len <= 1 -- add conditional that just ignores adding new bounds for regridded output datasets, fix related tests - Fix accidentally calling save plots and metrics twice in `_get_metrics_by_region()` - Fix failing integration tests pass in CI/CD - Refactor `test_diags.py` -- replace unittest with pytest - Refactor `test_all_sets.py` -- replace unittest with pytest - Test climatology datasets -- tested with 3d variables using `test_all_sets.py` CDAT Migration Phase 2: Refactor utilities and CoreParameter methods for reusability across diagnostic sets (#746) - Move driver type annotations to `type_annotations.py` - Move `lat_lon_driver._save_data_metrics_and_plots()` to `io.py` - Update `_save_data_metrics_and_plots` args to accept `plot_func` callable - Update `metrics.spatial_avg` to return an optionally `xr.DataArray` with `as_list=False` - Move `parameter` arg to the top in `lat_lon_plot.plot` - Move `_set_param_output_attrs` and `_set_name_yr_attrs` from `lat_lon_driver` to `CoreParameter` class CDAT Migration Phase 2: Refactor `zonal_mean_2d()` and `zonal_mean_2d_stratosphere()` sets (#774) CDAT Migration Phase 2: Refactor `qbo` set (#826)

tomvothecoder marked this pull request as draft May 14, 2024 18:25

tomvothecoder self-assigned this May 14, 2024

tomvothecoder added the cdat-migration-fy24 CDAT Migration FY24 Task label May 14, 2024

tomvothecoder added this to the FY24 Q3 (04/01/24 - 06/30/24) milestone May 14, 2024

tomvothecoder changed the title ~~CDAT Migration Phase 2: Refactor core utilities and lat_lon set (#677)~~ [Refactor]: CDAT Migration: cdms2.open slice flag adds additional time coordinate which affects downstream time series operations and diffs with Xarray code #759 May 14, 2024

tomvothecoder added 2 commits May 30, 2024 14:00

Add test scripts

add0a30

Add function to replicate "ccb" slice flag

6028810

tomvothecoder force-pushed the refactor/759-slice-flag branch from 59466c4 to 6028810 Compare May 30, 2024 21:00

Remove unused files

097ed96

tomvothecoder changed the title ~~[Refactor]: CDAT Migration: cdms2.open slice flag adds additional time coordinate which affects downstream time series operations and diffs with Xarray code #759~~ [Refactor]: Validate fix in PR #750 for #759 May 30, 2024

tomvothecoder added 6 commits May 30, 2024 15:27

Remove unused code

c6f4ad8

Fix incorrect xarray objects passed for regional subsetting

326a727

Remove unused comment

685134a

Add scripts to debug trefht

9a42b0e

Add scripts and files for debugging trefht

5d29bee

Add description at the top of debug script

0ec3cc3

tomvothecoder commented Jun 3, 2024

View reviewed changes

Add comment about results

8d714bc

tomvothecoder marked this pull request as ready for review June 3, 2024 21:07

tomvothecoder commented Jun 3, 2024

View reviewed changes

tomvothecoder mentioned this pull request Jun 3, 2024

[Bug]: Figure out why TREFHT regridded reference variable is not regridding on main with ESMF bilinear #817

Closed

tomvothecoder merged commit bf28fe2 into cdat-migration-fy24 Jun 3, 2024
4 checks passed

tomvothecoder deleted the refactor/759-slice-flag branch June 3, 2024 21:15

tomvothecoder mentioned this pull request Jun 3, 2024

[Refactor]: CDAT Migration: cdms2.open slice flag adds additional time coordinate which affects downstream time series operations and diffs with Xarray code #759

Closed

2 tasks

tomvothecoder added a commit that referenced this pull request Jul 15, 2024

[Refactor]: Validate fix in PR #750 for #759 (#815)

8b90a38

tomvothecoder added a commit that referenced this pull request Aug 21, 2024

[Refactor]: Validate fix in PR #750 for #759 (#815)

815450f

tomvothecoder added a commit that referenced this pull request Oct 1, 2024

[Refactor]: Validate fix in PR #750 for #759 (#815)

da6d141

tomvothecoder added a commit that referenced this pull request Oct 25, 2024

[Refactor]: Validate fix in PR #750 for #759 (#815)

c94d2e2

tomvothecoder added a commit that referenced this pull request Oct 29, 2024

[Refactor]: Validate fix in PR #750 for #759 (#815)

cb00450

tomvothecoder added a commit that referenced this pull request Oct 29, 2024

[Refactor]: Validate fix in PR #750 for #759 (#815)

0089118

tomvothecoder added a commit that referenced this pull request Dec 4, 2024

[Refactor]: Validate fix in PR #750 for #759 (#815)

08f9cee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor]: Validate fix in PR #750 for #759 #815

[Refactor]: Validate fix in PR #750 for #759 #815

tomvothecoder commented May 14, 2024 •

edited

Loading

tomvothecoder left a comment •

edited

Loading

tomvothecoder Jun 3, 2024 •

edited

Loading

tomvothecoder Jun 3, 2024 •

edited

Loading

tomvothecoder Jun 3, 2024

tomvothecoder commented Jun 11, 2024 •

edited

Loading

chengzhuzhang commented Jun 13, 2024

	# use nlat to decide data resolution, higher number means higher data
	# resolution. For the difference plot, regrid toward lower resolution
	if len(axes1[1]) <= len(axes2[1]):
	mv_grid = mv1.getGrid()
	mv1_reg = mv1
	mv2_reg = mv2.regrid(
	mv_grid, regridTool=regrid_tool, regridMethod=regrid_method
	)
	mv2_reg.units = mv2.units

[Refactor]: Validate fix in PR #750 for #759 #815

[Refactor]: Validate fix in PR #750 for #759 #815

Conversation

tomvothecoder commented May 14, 2024 • edited Loading

Description

5/30/24

Checklist

tomvothecoder left a comment • edited Loading

Choose a reason for hiding this comment

tomvothecoder Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

tomvothecoder Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

tomvothecoder Jun 3, 2024

Choose a reason for hiding this comment

tomvothecoder commented Jun 11, 2024 • edited Loading

chengzhuzhang commented Jun 13, 2024

tomvothecoder commented May 14, 2024 •

edited

Loading

tomvothecoder left a comment •

edited

Loading

tomvothecoder Jun 3, 2024 •

edited

Loading

tomvothecoder Jun 3, 2024 •

edited

Loading

tomvothecoder commented Jun 11, 2024 •

edited

Loading