CDAT Migration: Refactor `polar` set

forsyth2 commented 10 months ago

Description

Refactor the polar set. Following the directions at https://github.com/E3SM-Project/e3sm_diags/wiki/CDAT-Migration-FY24-%E2%80%90-General-Guide#getting-started:

git fetch upstream
git checkout cdat-migration-fy24
git checkout -b refactor/659-polar
git branch -m refactor/659-polar-set # Include "set" in name
mamba env create -f conda-env/dev.yml -n e3sm_diags_dev_659
mamba activate e3sm_diags_dev_659 # Says to use conda
conda activate e3sm_diags_dev_659
python -m pip install .

Closes #659

Summary of Changes:

polar_driver.py and polar_plot.py

Refactor to use Xarray/xCDAT based modules

driver/utils/regrid.py

Update subset_and_align_datasets() to make copies of dataset objects before performing computations for cleaner code and ease of debugging
Fix _subset_on_region() not subsetting specific regions correctly due to order of subsetting tuple. This function makes sure that the axes/axes being subsetted on is in ascending order. If it is not in ascending order, some slice specifications might have two negative values (e.g., (-90, -55) for 'polar_S') which can result in the subset incorrectly excluding of values that seem like they don't fit within the slice slice spec range when they actually are (circular coordinates).

plot/utils.py

Add optional rect arg to _add_colorbar() for adjustment of the rect used for creating colorbar, along with _get_rect(), which can vary based on the set
Add optional left_text_pos and right_text_pos args to _add_min_mean_max_test() and add_rmse_and_corr_text() for adjustment of the x and y position of each respective text, which can vary based on the set

cosp_histogram_py

Update variable and function names for consistency with other refactored plotters.
Replace block of code for writing out plots with _save_plot()

zonal_mean_2d_plot.py

Remove extraneous "#" character

auxiliary_tools/cdat_regression_testing/template_run_script.py

Update incorrect chown command with chmod.

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)

forsyth2 commented 10 months ago

Using this script:

diags_set=polar

cd e3sm_diags
# https://anaconda.org/cdat/repo?sort=_name&sort_order=asc
for cdat_package in cd77 cdat cdat_compute_graph cdat_info cdat_jupyter_vcdat cdms2 cdp cdtime cdutil cibots compute_graph django-allow-cidr dv3d e3sm_nex esgf-compute-api esgf-compute-cert esgf-search ezget genutil image-compare jupyter-vcdat jupyterlab-git lats libcdms lmoments mesalib myproxyclient output_viewer selenium_testlib testsrunner thermo toolchain vcdat vcs vcs-js vcsaddons vtk vtk-cdat wk xmgrace
do
    git grep -n ${cdat_package} e3sm_diags/driver/${diags_set}_driver.py > /dev/null
    if [ $? == 0 ]; then
    echo ""
    echo "git grep -n ${cdat_package} e3sm_diags/driver/${diags_set}_driver.py"
    git grep -n ${cdat_package} e3sm_diags/driver/${diags_set}_driver.py
    fi
    git grep -n ${cdat_package} e3sm_diags/plot/cartopy/${diags_set}_plot.py > /dev/null
    if [ $? == 0 ]; then
    echo ""
    echo "git grep -n ${cdat_package} e3sm_diags/plot/cartopy/${diags_set}_plot.py"
    git grep -n ${cdat_package} e3sm_diags/plot/cartopy/${diags_set}_plot.py
    fi
done

I get:

git grep -n cdms2 e3sm_diags/driver/polar_driver.py
e3sm_diags/driver/polar_driver.py:6:import cdms2
e3sm_diags/driver/polar_driver.py:74:            with cdms2.open(mask_path) as f:
e3sm_diags/driver/polar_driver.py:94:                # This is cdms2 fix for bad mask, Denis' fix should fix this.
e3sm_diags/driver/polar_driver.py:98:                # This is cdms2 fix for bad mask, Denis' fix should fix this.
e3sm_diags/driver/polar_driver.py:101:                # This is cdms2 fix for bad mask, Denis' fix should fix this.

forsyth2 commented 10 months ago

@chengzhuzhang @tomvothecoder Are the testing directions at https://e3sm-project.github.io/e3sm_diags/_build/html/main/dev_guide/testing.html up to date? I tried running ./tests/test.sh on the main branch and got:

================================================================================================== short test summary info ==================================================================================================
FAILED tests/integration/test_diags.py::TestAllSets::test_lat_lon - AssertionError: Lists differ: ['ERA-Interim-T-850-ANN-global.png'] != []
FAILED tests/integration/test_diags.py::TestAllSets::test_lat_lon_regional - AssertionError: Lists differ: ['ERA-Interim-T-850-ANN-CONUS_RRM.png'] != []
FAILED tests/integration/test_diags.py::TestAllSets::test_polar - AssertionError: Lists differ: ['ERA-Interim-T-850-ANN-polar_S.png'] != []
FAILED tests/integration/test_diags.py::TestAllSets::test_zonal_mean_2d - AssertionError: Lists differ: ['ERA-Interim-T-ANN-global.png'] != []
=================================================================================== 4 failed, 23 passed, 22 warnings in 351.33s (0:05:51) ===================================================================================

tomvothecoder commented 9 months ago

@chengzhuzhang @tomvothecoder Are the testing directions at e3sm-project.github.io/e3sm_diags/_build/html/main/dev_guide/testing.html up to date? I tried running ./tests/test.sh on the main branch and got:

================================================================================================== short test summary info ==================================================================================================
FAILED tests/integration/test_diags.py::TestAllSets::test_lat_lon - AssertionError: Lists differ: ['ERA-Interim-T-850-ANN-global.png'] != []
FAILED tests/integration/test_diags.py::TestAllSets::test_lat_lon_regional - AssertionError: Lists differ: ['ERA-Interim-T-850-ANN-CONUS_RRM.png'] != []
FAILED tests/integration/test_diags.py::TestAllSets::test_polar - AssertionError: Lists differ: ['ERA-Interim-T-850-ANN-polar_S.png'] != []
FAILED tests/integration/test_diags.py::TestAllSets::test_zonal_mean_2d - AssertionError: Lists differ: ['ERA-Interim-T-ANN-global.png'] != []
=================================================================================== 4 failed, 23 passed, 22 warnings in 351.33s (0:05:51) ===================================================================================

I don't recall any changes to how tests are run on the main or the cdat-migration-fy24 branches. I'm not sure why these tests are failing on main.

tomvothecoder commented 9 months ago

I recently squashed the cdat-migration-fy24 branch to a few commits and just rebased this branch on top of it.

Can you make sure to checkout the latest version of his branch? I also recommend stashing and reapplying any changes you might have.