CDAT Migration Phase 2: Refactor `enso_diags` set #832

tomvothecoder · 2024-07-31T22:27:03Z

Description

Closes CDAT Migration Phase 2: Refactor enso_diags set #663

TODO

8/14/24

Refactor map driver
Add optional kwargs plot_kwargs and viewer_descr to _save_data_metrics_and_plots() to support custom enso diags configs
Refactor map plotter
- Get it working for a variable with saved outputs
- Consider refactoring portions of the code to reuse general plotter functions
Refactor scatter driver
Refactor scatter plotter

8/20/24

Regression testing with all variables
~~"ccb" not being considered for regional subsetting "20S20N" --> 2 extra points causes diffs in floating points and plots.~~ won't fix based on Jill's comment below

8/21/24

Fix unit test
Fix integration test
- OSError: No files found for target variable SST or derived variables ([('sst',), ('TS', 'OCNFRAC'), ('SST',)]) in tests/integration/integration_test_data. -- due to incorrect exception
- AttributeError: Either "parameter.short_ref_name", "parameter.reference_name", or "parameter.ref_name" must be set to get the name and years attribute for reference datasets. -- need to preserve the else statement from the previous logic that sets the test and ref names to parameter.test_name/ parameter.ref_name regardless if either is set or not (empty string).

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 · 2024-08-20T21:14:55Z

@chengzhuzhang I'm running into the same issue found in #664 where the "ccb" flag adds an additional point to the start and end of the lat coordinates, resulting in mismatching output arrays. Once I remove the two additional points, the variables align between main and this branch.

I'm trying to write a adapt the function I wrote to handle the "ccb" flag for the lat dimension. It is not clear how cdms2 is calculating the additional start and end points (delta between bounds does not seem to be used unlike "ccb" with time coordinates). I'll need to investigate the code in cdms2 unfortunately. UPDATE: Here's the function in cdms2. I'm not keen on trying to understand this really complex implementation.

Any thoughts?

tomvothecoder · 2024-08-20T21:17:17Z

@chengzhuzhang What is the purpose of using the "ccb" flag in the CDAT code? I'm not clear on the reasoning.

chengzhuzhang · 2024-08-20T21:22:04Z

@tomvothecoder I think it is just a convenience choice to use ccb in this set. I don't think it is important to exactly matching the number of lat lon points in refactored code to the main.

To support flag like ccb can be investigated in xcdat, but here I don't see it is necessary to have an exact match.

tomvothecoder · 2024-08-21T18:43:55Z

I think it is just a convenience choice to use ccb in this set.

@chengzhuzhang What I mean is: what is the purpose of the CDAT slice flag? I'm not clear on why we need the extra coordinates points when opening time series datasets using ccb.

I don't think it is important to exactly matching the number of lat lon points in refactored code to the main.

Got it

chengzhuzhang · 2024-08-21T18:54:17Z

@tomvothecoder could you point me to the original codes where ccb are used, is it used for both time and lat/lon dimensions?

Co-authored-by: Tom Vo <tomvothecoder@gmail.com>

* 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 <tomvothecoder@gmail.com>

- Use new linear regression function `calc_linear_regression` which uses xskillscore and produces same results as old function - Update variable names to denote xr.Dataset objects - Update `_create_metrics_dict()` arguments - Rename `calculate_nino_index()` to `calculate_nino_index_obs()` - Extract `_get_contour_levels()` from `run_diags_map()` - Add `_align_time_coords()` to align nino index time coords to variable time coords

- Add optional kwarg `c_label_fmt_and_pad_func` to `_add_colorbar()`

…end slice

- Update function to update start or end slice correctly - Add regression testing notebooks

- Write out results of of scatter driver to netCDF - Fix incorrect units in `qflx_convert_to_lhflx`

tomvothecoder · 2024-08-21T19:49:36Z

@chengzhuzhang This PR should be good to go after the build passes. Let me know if you have any comments or questions based on my testing results.

Regression testing results (netCDF) -- should be good to go

Regression-coefficient -- within rtol once the extra two lat coordinates are removed from the CDAT files (added via "ccb" slice flag)
Feedback -- rel diffs are large because we are comparing anomalies. I compared the min, max, mean, and std and they are all pretty close for one file. I think if I compared the spatial average before calculating anomalies instead then the rtol would be met. Note: CDAT code on main doesn't save these variables out to netCDF so I made a temporary manual change for the comparison.

Regression test (plots) -- any diffs are not discernible via eye-check

main branch: https://portal.nersc.gov/project/e3sm/cdat-migration-fy24/main/viewer/enso_diags/index.html
dev branch: https://portal.nersc.gov/project/e3sm/cdat-migration-fy24/663-enso-diags/viewer/enso_diags/index.html

Regression-coefficient -- mostly the same, except there is a small white bar for the xCDAT plots (I think due to the missing 2 lat points). Example diff for SHFLX on main vs. dev branch
Feedback -- mostly the same, except data points and lines are moved over just a very small amount because of the floating point diffs after the subsetting, spatial averaging, then anomalies calculation (I think due to the missing 2 lat points). Example for SHFLX main vs. dev branch.

tomvothecoder · 2024-08-21T19:56:40Z

@tomvothecoder could you point me to the original codes where ccb are used, is it used for both time and lat/lon dimensions?

In addition to time when opening up time series, ccb is used in default_regions.py for regional subsetting. Example:

e3sm_diags/e3sm_diags/derivations/default_regions.py

Lines 132 to 135 in b77d681

    
           "NINO3": { 
        
               "domain": cdutil.region.domain( 
        
                   latitude=(-5.0, 5.0, "ccb"), longitude=(210.0, 270.0, "ccb") 
        
               )

chengzhuzhang · 2024-08-21T20:02:00Z

Thanks, @tomvothecoder . Okay, it is for the NINO index. I think as long as the refactored code is in line with the xCDAT example Steve Po-Chedley provided (https://xcdat.readthedocs.io/en/latest/examples/spatial-average.html#4.-Nino-3.4-Region). I think we should be good.

- Update `_get_test_name()` and `_get_ref_name()` to preserve the else statement from the CDAT logic

chengzhuzhang · 2024-08-22T18:31:57Z

@tomvothecoder I think there is a problem with the metrics (max, min, mean) printed in wrong order in the refactored code. Also I can't seem to find other plots, only Regression-coefficient plot for PRECT is available in the dev branch results...

tomvothecoder · 2024-08-22T22:56:16Z

@tomvothecoder I think there is a problem with the metrics (max, min, mean) printed in wrong order in the refactored code. Also I can't seem to find other plots, only Regression-coefficient plot for PRECT is available in the dev branch results...

I ran the run script again. All of them are there now: https://portal.nersc.gov/project/e3sm/cdat-migration-fy24/663-enso-diags/viewer/enso_diags/index.html

Thanks for catching the issue with the max and min values needing to be swapped. I also noticed RMSE and CORR aren't printed. Those should be easy fixes and will be addressed via #838.

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 <tomvothecoder@gmail.com> 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 <tomvothecoder@gmail.com> 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 <tomvothecoder@gmail.com> [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 <tomvothecoder@gmail.com> 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 <tomvothecoder@gmail.com> 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 added this to the FY24 Q4 (07/01/24 - 9/30/24) milestone Jul 31, 2024

tomvothecoder self-assigned this Jul 31, 2024

tomvothecoder force-pushed the cdat-migration-fy24 branch from 68ede2f to e0e3bab Compare August 21, 2024 18:58

chengzhuzhang and others added 14 commits August 21, 2024 12:00

Refactor 654 zonal mean xy (#752)

4efe291

Co-authored-by: Tom Vo <tomvothecoder@gmail.com>

Start refactoring enso_diags driver

f19bc99

Add baseline test structure

c9d2e65

Refactor perform_regression

8d3ab6e

Refactor map plotter

dfbc4ae

Refactor map plotter to reuse general plotter functions

6a361f9

- Add optional kwarg `c_label_fmt_and_pad_func` to `_add_colorbar()`

Update _get_slice_with_bounds() to handle "ccb" flag for start and …

5b71c61

…end slice

Update _get_slice_with_bounds()

ab0e81a

- Update function to update start or end slice correctly - Add regression testing notebooks

Refactor scatter driver and plotter

402c7ce

Update docstrings

8f4ab40

Fix missing title for scatter plot

210d6ac

- Write out results of of scatter driver to netCDF - Fix incorrect units in `qflx_convert_to_lhflx`

Fix unit tests

469da6e

tomvothecoder force-pushed the refactor/663-enso-diags branch from d9674a6 to 469da6e Compare August 21, 2024 19:01

tomvothecoder added 2 commits August 21, 2024 12:07

Revert accidental rebase changes

719fc76

Update type annoations

891cea2

tomvothecoder marked this pull request as ready for review August 21, 2024 19:15

tomvothecoder added 3 commits August 21, 2024 12:23

Fix circular import

ef39fce

Fix TypeError for type annotation

ae6fa6c

Fix type annotation

19a060f

tomvothecoder added 5 commits August 21, 2024 13:10

Fix incorrect exception being caught in calculate_nino_index_model()

af9bf06

Fix error message being caught

4f73809

Fix AttributeError with ref_name_yrs

b9ee8cd

Fix invalid var reference

96d6238

Fix integration test failing due to AttributeError with ref name

bd950f1

- Update `_get_test_name()` and `_get_ref_name()` to preserve the else statement from the CDAT logic

tomvothecoder merged commit defce25 into cdat-migration-fy24 Aug 22, 2024
4 checks passed

tomvothecoder deleted the refactor/663-enso-diags branch August 22, 2024 17:19

tomvothecoder mentioned this pull request Aug 22, 2024

CDAT Migration Phase 2: Refactor enso_diags set #663

Closed

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

CDAT Migration Phase 2: Refactor enso_diags set (#832)

bc4709b

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

CDAT Migration Phase 2: Refactor enso_diags set (#832)

75122b2

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

CDAT Migration Phase 2: Refactor enso_diags set (#832)

3649288

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

CDAT Migration Phase 2: Refactor enso_diags set (#832)

dda0bd8

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

CDAT Migration Phase 2: Refactor enso_diags set (#832)

820a68f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDAT Migration Phase 2: Refactor `enso_diags` set #832

CDAT Migration Phase 2: Refactor `enso_diags` set #832

tomvothecoder commented Jul 31, 2024 •

edited

Loading

tomvothecoder commented Aug 20, 2024 •

edited

Loading

tomvothecoder commented Aug 20, 2024 •

edited

Loading

chengzhuzhang commented Aug 20, 2024

tomvothecoder commented Aug 21, 2024

chengzhuzhang commented Aug 21, 2024

tomvothecoder commented Aug 21, 2024 •

edited

Loading

tomvothecoder commented Aug 21, 2024 •

edited

Loading

chengzhuzhang commented Aug 21, 2024

chengzhuzhang commented Aug 22, 2024 •

edited

Loading

tomvothecoder commented Aug 22, 2024

CDAT Migration Phase 2: Refactor enso_diags set #832

CDAT Migration Phase 2: Refactor enso_diags set #832

Conversation

tomvothecoder commented Jul 31, 2024 • edited Loading

Description

TODO

Checklist

tomvothecoder commented Aug 20, 2024 • edited Loading

tomvothecoder commented Aug 20, 2024 • edited Loading

chengzhuzhang commented Aug 20, 2024

tomvothecoder commented Aug 21, 2024

chengzhuzhang commented Aug 21, 2024

tomvothecoder commented Aug 21, 2024 • edited Loading

Regression testing results (netCDF) -- should be good to go

Regression test (plots) -- any diffs are not discernible via eye-check

tomvothecoder commented Aug 21, 2024 • edited Loading

chengzhuzhang commented Aug 21, 2024

chengzhuzhang commented Aug 22, 2024 • edited Loading

tomvothecoder commented Aug 22, 2024

CDAT Migration Phase 2: Refactor `enso_diags` set #832

CDAT Migration Phase 2: Refactor `enso_diags` set #832

tomvothecoder commented Jul 31, 2024 •

edited

Loading

tomvothecoder commented Aug 20, 2024 •

edited

Loading

tomvothecoder commented Aug 20, 2024 •

edited

Loading

tomvothecoder commented Aug 21, 2024 •

edited

Loading

tomvothecoder commented Aug 21, 2024 •

edited

Loading

chengzhuzhang commented Aug 22, 2024 •

edited

Loading