Fix `convert_units()` not keeping attrs #916

tomvothecoder · 2025-01-16T22:07:24Z

Description

Closes [Bug]: Re-testing land variables for unit conversion issue on main #915

Fix convert_units() not keeping attrs
Update convert_units() to support additional non-CF compliant unit conversions
- Includes "gC/m^2" to "kgC/m^2" and gC/m^2/s to kgC/m^2/s
Add docstrings to convert_units() docstring
Add non-CF compliant unit conversion to convert_units()
Delete contour_levels for QIRRIG_GRND, QIRRIG_REAL, and QIRRIG_SURF
- The contour levels were defined as contour_levels = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0], which matplotlib to raise ValueError: Contour levels must be increasing

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 · 2025-01-16T22:07:54Z

Hey @forsyth2 and/or @chengzhuzhang, can you try re-testing land variables in zppy using this branch? This PR should fix the missing units attribute in convert_units().

e3sm_diags/derivations/utils.py

tomvothecoder · 2025-01-16T22:13:35Z

New error:

2025-01-16 16:11:06,538 [ERROR]: core_parameter.py(_run_diag:343) >> Error in e3sm_diags.driver.lat_lon_land_driver
Traceback (most recent call last):
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/parameter/core_parameter.py", line 340, in _run_diag
    single_result = module.run_diag(self)
                    ^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/lat_lon_land_driver.py", line 13, in run_diag
    return base_run_diag(parameter)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/lat_lon_driver.py", line 75, in run_diag
    ds_test = test_ds.get_climo_dataset(var_key, season)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/utils/dataset_xr.py", line 401, in get_climo_dataset
    ds = self._get_climo_dataset(season)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/utils/dataset_xr.py", line 430, in _get_climo_dataset
    ds = self._get_dataset_with_derived_climo_var(ds)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/utils/dataset_xr.py", line 720, in _get_dataset_with_derived_climo_var
    ds_derived = self._get_dataset_with_derivation_func(
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/utils/dataset_xr.py", line 935, in _get_dataset_with_derivation_func
    derived_var = func(*func_args)  # pragma: nocover
                  ^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/derivations/derivations.py", line 1185, in <lambda>
    "SR": OrderedDict([(("SR",), lambda v: convert_units(v, target_units="kgC/m^2"))]),
                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/derivations/utils.py", line 92, in convert_units
    original_udunit = cf_units.Unit(var_new.attrs["units"])
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/cf_units/__init__.py", line 756, in __init__
    raise value_error from None
ValueError: [UT_UNKNOWN] Failed to parse unit "gC/m^2/s"

@chengzhuzhang If cf_units does not support gC/m^2/s to kgC/m^2/s conversion, we may need to update convert_units() to manually support this. I think the below should work?

        elif var_new.attrs["units"] == "gC/m^2/s" and target_units == "kgC/m^2/s":
            var_new = var_new / 1000.0
            var_new.attrs["units"] = target_units
        else:
            original_udunit = cf_units.Unit(var_new.attrs["units"])
            target_udunit = cf_units.Unit(target_units)

            var_new.values = original_udunit.convert(var_new.values, target_udunit)
            var_new.attrs["units"] = target_units

chengzhuzhang · 2025-01-16T22:33:40Z

```python
@chengzhuzhang If cf_units does not support gC/m^2/s to kgC/m^2/s conversion, we may need to update convert_units() to manually support this. I think the below should work?

Yes... these units are not cf compliant and have to be manually converted.

…conversions - Includes "gC/m^2" to "kgC/m^2" and gC/m^2/s to kgC/m^2/s

forsyth2 · 2025-01-16T22:55:33Z

can you try re-testing land variables in zppy using this branch?

This is post-CDAT migration, correct? In that case, I'll test after I finish testing zppy's main with the pre-CDAT Diags.

tomvothecoder · 2025-01-16T23:09:55Z

can you try re-testing land variables in zppy using this branch?

This is post-CDAT migration, correct? In that case, I'll test after I finish testing zppy's main with the pre-CDAT Diags.

Yes that's correct.

The run script @chengzhuzhang posted in #915 now completes without unit conversion issues.

chengzhuzhang

@tomvothecoder Thank you for a quick fix!
I'm comparing your results with mine (which ran with e3sm_diags v2): Your results missing some QIRRIG_* variables:
from Your results:

QIRRIG_ORIG global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_WM global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan

While I have:

QIRRIG_GRND global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_ORIG global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_REAL global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_SURF global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_WM global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan

Other variables show consistent results.
I'm trying to understand why 3/5 QIRRIG_* variables are missing.

tomvothecoder · 2025-01-16T23:28:23Z

@tomvothecoder Thank you for a quick fix! I'm comparing your results with mine (which ran with e3sm_diags v2): Your results missing some QIRRIG_* variables: from Your results:
QIRRIG_ORIG global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_WM global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
While I have:
QIRRIG_GRND global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_ORIG global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_REAL global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_SURF global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
QIRRIG_WM global	mm/day	0.0	0.0	0.0	0.0	0.0	0.0	nan
Other variables show consistent results. I'm trying to understand why 3/5 QIRRIG_* variables are missing.

I executed the run_script with just QIRRIG_GRND, QIRRIG_REAL, and QIRRIG_SURF. It fails with the matplotlib error ValueError: Contour levels must be increasing. I think the root cause is that in lat_lon_land_model_vs_model.cfg, those variables define contour_levels = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0].

e3sm_diags/e3sm_diags/driver/default_diags/lat_lon_land_model_vs_model.cfg

Lines 424 to 433 in d01fd7a

    
           [#] 
        
           sets = ["lat_lon_land"] 
        
           case_id = "model_vs_model" 
        
           variables = ["QIRRIG_GRND"] 
        
           seasons = ["ANN", "DJF", "MAM", "JJA", "SON"] 
        
           regions = ["global"] 
        
           test_colormap = "WhiteBlueGreenYellowRed.rgb" 
        
           reference_colormap = "WhiteBlueGreenYellowRed.rgb" 
        
           diff_colormap = "BrBG" 
        
           contour_levels = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]

The odd part is that it was working fine before with v2?

Related stacktrace

Traceback (most recent call last):
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/parameter/core_parameter.py", line 340, in _run_diag
    single_result = module.run_diag(self)
                    ^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/lat_lon_land_driver.py", line 13, in run_diag
    return base_run_diag(parameter)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/lat_lon_driver.py", line 110, in run_diag
    _run_diags_2d(
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/lat_lon_driver.py", line 349, in _run_diags_2d
    _save_data_metrics_and_plots(
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/driver/utils/io.py", line 101, in _save_data_metrics_and_plots
    plot_func(**args)
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/plot/lat_lon_plot.py", line 74, in plot
    _add_colormap(
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/plot/lat_lon_plot.py", line 203, in _add_colormap
    contour_plot = _add_contour_plot(
                   ^^^^^^^^^^^^^^^^^^
  File "/gpfs/fs1/home/ac.tvo/E3SM-Project/e3sm_diags/e3sm_diags/plot/utils.py", line 232, in _add_contour_plot
    c_plot = ax.contourf(
             ^^^^^^^^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/cartopy/mpl/geoaxes.py", line 306, in wrapper
    return func(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/cartopy/mpl/geoaxes.py", line 350, in wrapper
    return func(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/cartopy/mpl/geoaxes.py", line 1646, in contourf
    result = super().contourf(*args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/matplotlib/__init__.py", line 1521, in inner
    return func(
           ^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/matplotlib/axes/_axes.py", line 6760, in contourf
    contours = mcontour.QuadContourSet(self, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/matplotlib/contour.py", line 708, in __init__
    kwargs = self._process_args(*args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/matplotlib/contour.py", line 1326, in _process_args
    x, y, z = self._contour_args(args, kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/matplotlib/contour.py", line 1377, in _contour_args
    self._process_contour_level_args(args, z.dtype)
  File "/home/ac.tvo/miniforge3/envs/e3sm_diags_dev_915/lib/python3.12/site-packages/matplotlib/contour.py", line 1037, in _process_contour_level_args
    raise ValueError("Contour levels must be increasing")
ValueError: Contour levels must be increasing

chengzhuzhang · 2025-01-16T23:36:54Z

The odd part is that it was working fine before with v2?

I guess newer version of matplotlib added a check for contour level probably. In this case, maybe let's just delete the cases with contour_levels = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]? Just to make sure plots and metrics can be generated.

…SURF` - The contour levels were defined as `contour_levels = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]`, which `matplotlib` to raise `ValueError: Contour levels must be increasing`

tomvothecoder · 2025-01-16T23:46:10Z

The odd part is that it was working fine before with v2?

I guess newer version of matplotlib added a check for contour level probably. In this case, maybe let's just delete the cases with contour_levels = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]? Just to make sure plots and metrics can be generated.

Thanks, that seemed to have done the trick. I think this PR is good to merge and then @forsyth2 can re-test with zppy later. I'll be able to proceed with #910 afterwards.

https://web.lcrc.anl.gov/public/e3sm/ac.tvo/zppy_weekly_comprehensive_v3_output/test_zppy_pr651_20250115/v3.LR.historical_0051/post/viewer/table-data-land/ANN_metrics_table.html

tomvothecoder · 2025-01-16T23:47:39Z

@chengzhuzhang I actually noticed that those three variables are missing plots in your results (the ANN links aren't clickable). This issue seemed to have existed before? In any case, the plots now generate here.

chengzhuzhang · 2025-01-16T23:51:41Z

@chengzhuzhang I actually noticed that those three variables are missing plots in your results (the ANN links aren't clickable). This issue seemed to have existed before? In any case, the plots now generate here.

Good catch! Deleting countour_levels at least can produce the figures now. I think this is good to merge.

Fix convert_units() not keeping attrs

2d85e25

tomvothecoder commented Jan 16, 2025

View reviewed changes

e3sm_diags/derivations/utils.py Outdated Show resolved Hide resolved

tomvothecoder marked this pull request as ready for review January 16, 2025 22:09

tomvothecoder added 2 commits January 16, 2025 16:37

Update convert_units() to support additional non-CF compliant unit …

e9b04ba

…conversions - Includes "gC/m^2" to "kgC/m^2" and gC/m^2/s to kgC/m^2/s

Add docstrings to convert_units() docstring

09d0b46

tomvothecoder added the bug Bug fix (will increment patch version) label Jan 16, 2025

tomvothecoder self-assigned this Jan 16, 2025

tomvothecoder requested review from chengzhuzhang and forsyth2 January 16, 2025 22:44

tomvothecoder added this to the FY25 Q2 (01/01/25 - 03/31/25) milestone Jan 16, 2025

tomvothecoder mentioned this pull request Jan 16, 2025

Bump to 3.0.0rc1 #910

Merged

12 tasks

Add non-CF compliant unit conversion to convert_units()

d490d18

chengzhuzhang reviewed Jan 16, 2025

View reviewed changes

Delete contour_levels for QIRRIG_GRND, QIRRIG_REAL, and QIRRIG_…

4bb097e

…SURF` - The contour levels were defined as `contour_levels = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]`, which `matplotlib` to raise `ValueError: Contour levels must be increasing`

chengzhuzhang approved these changes Jan 16, 2025

View reviewed changes

tomvothecoder merged commit 926724f into main Jan 16, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `convert_units()` not keeping attrs #916

Fix `convert_units()` not keeping attrs #916

tomvothecoder commented Jan 16, 2025 •

edited

Loading

tomvothecoder commented Jan 16, 2025 •

edited

Loading

tomvothecoder commented Jan 16, 2025 •

edited

Loading

chengzhuzhang commented Jan 16, 2025

forsyth2 commented Jan 16, 2025

tomvothecoder commented Jan 16, 2025

chengzhuzhang left a comment •

edited

Loading

tomvothecoder commented Jan 16, 2025

chengzhuzhang commented Jan 16, 2025

tomvothecoder commented Jan 16, 2025

tomvothecoder commented Jan 16, 2025 •

edited

Loading

chengzhuzhang commented Jan 16, 2025

Fix convert_units() not keeping attrs #916

Fix convert_units() not keeping attrs #916

Conversation

tomvothecoder commented Jan 16, 2025 • edited Loading

Description

Checklist

tomvothecoder commented Jan 16, 2025 • edited Loading

tomvothecoder commented Jan 16, 2025 • edited Loading

chengzhuzhang commented Jan 16, 2025

forsyth2 commented Jan 16, 2025

tomvothecoder commented Jan 16, 2025

chengzhuzhang left a comment • edited Loading

Choose a reason for hiding this comment

tomvothecoder commented Jan 16, 2025

Related stacktrace

chengzhuzhang commented Jan 16, 2025

tomvothecoder commented Jan 16, 2025

tomvothecoder commented Jan 16, 2025 • edited Loading

chengzhuzhang commented Jan 16, 2025

Fix `convert_units()` not keeping attrs #916

Fix `convert_units()` not keeping attrs #916

tomvothecoder commented Jan 16, 2025 •

edited

Loading

tomvothecoder commented Jan 16, 2025 •

edited

Loading

tomvothecoder commented Jan 16, 2025 •

edited

Loading

chengzhuzhang left a comment •

edited

Loading

tomvothecoder commented Jan 16, 2025 •

edited

Loading