Refactor 654 zonal mean xy

chengzhuzhang commented 10 months ago

Description

Closes #654 to refactor zonal mean xy to use xcdat/xarray

Checklist

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[ ] My changes generate no new warnings: there is warning from xgcm
[x] 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)
[x] I have added tests that prove my fix is effective or that my feature works
[x] 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)

chengzhuzhang commented 10 months ago

The components to refactor include the driver and the plotter. File 1, zonal_mean_xy_driver.py

[x] refactor regrid_to_lower_res_1d function that regrids 1-D transient variable toward lower resolution of two variables . Question can I reuse or extend align_grids_to_lower_res.

File 2, zonal_mean_xy_plot.py

[x] use xarray object instead of cdms transient variable. No major function to refactor.

chengzhuzhang commented 9 months ago

With the new commit https://github.com/E3SM-Project/e3sm_diags/pull/752/commits/341ac3cf1fba10a1ceff5eab18f012d89749f90f, there are two problems:

an error:

2023-12-01 16:17:54,212 [ERROR]: core_parameter.py(_run_diag:326) >> Error in e3sm_diags.driver.zonal_mean_xy_driver
Traceback (most recent call last):
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/parameter/core_parameter.py", line 323, in _run_diag
single_result = module.run_diag(self)
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/driver/zonal_mean_xy_driver.py", line 88, in run_diag
_run_diags_3d(
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/driver/zonal_mean_xy_driver.py", line 200, in _run_diags_3d
ds_test_rg = regrid_z_axis_to_plevs(ds_test, var_key, parameter.plevs)
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/driver/utils/regrid.py", line 384, in regrid_z_axis_to_plevs
ds_plevs = _hybrid_to_plevs(ds, var_key, plevs)
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/driver/utils/regrid.py", line 443, in _hybrid_to_plevs
result = ds.regridder.vertical(
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xcdat/regridder/accessor.py", line 392, in vertical
input_grid = _get_input_grid(
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xcdat/regridder/accessor.py", line 445, in _get_input_grid
grid = input_grid.regridder.grid
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xcdat/regridder/accessor.py", line 91, in grid
data, bnds = self._get_axis_data(axis)
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xcdat/regridder/accessor.py", line 111, in _get_axis_data
_validate_grid_has_single_axis_dim(name, coord_var)
File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xcdat/regridder/grid.py", line 744, in _validate_grid_has_single_axis_dim
raise ValueError(
ValueError: Multiple 'X' axis dims were found in this dataset, ['lon', 'slon']. Please drop the unused dimension(s) before performing grid operations.

pre-commiting hooks isort and black conflicts. And updating .pre-committing yaml didn't help.

chengzhuzhang commented 9 months ago

@tomvothecoder thank you for fixing this branch!

For the excessive slat, slon, it turned out to be they are included in the model climatology files, the relevant texts in the header as shown below:

        double slat(slat) ;
                slat:long_name = "Latitude for staggered FV grid" ;
                slat:units = "degrees_north" ;
        double w_stag(slat) ;
                w_stag:long_name = "Latitude weights for staggered FV grid" ;
        double slon(slon) ;
                slon:long_name = "Longitude for staggered FV grid" ;
                slon:units = "degrees_east" ;

I think I will try dropping the slat and slon dims, as a workaround for now.

chengzhuzhang commented 9 months ago

Hey @chengzhuzhang, I addressed your recent issues in this comment.

I also rebased this branch on the latest version of cdat-migration-fy24, which I squashed down into less commits. If you have any un-committed changes, can you stash then, check out the latest version of this branch, then pop them back on top? Thanks

yep, I have checked out the latest version. Thanks for the heads-up!

chengzhuzhang commented 9 months ago

@tomvothecoder I made plotting work, but will try consolidate the codes a little better. But it seems my pre-commit isort still has errorsthat I can't sort out..

chengzhuzhang commented 9 months ago

Other than mypy complaints. There are still several problems causing some variables fails to generate plots. The log for a full run. The result can be found here

tomvothecoder commented 8 months ago

Other than mypy complaints. There are still several problems causing some variables fails to generate plots. The log for a full run. The result can be found here

I listed the errors from the log below:

Issue 1

  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/driver/zonal_mean_xy_driver.py", line 322, in regrid_to_lower_res_1d
    if len(ds_test_1d.lat) > len(ds_ref_1d.lat):
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/common.py", line 278, in __getattr__
    raise AttributeError(
AttributeError: 'DataArray' object has no attribute 'lat'

The name of the latitude coordinates is not lat on the DataArray. You should use xc.get_dim_keys(DataArray, axis="Y") to get the name dynamically.

Issue 2

packages/e3sm_diags/plot/zonal_mean_xy_plot.py:115: RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (`matplotlib.pyplot.figure`) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam `figure.max_open_warning`). Consider using `matplotlib.pyplot.close()`.
  fig = plt.figure(figsize=parameter.figsize, dpi=parameter.dpi)

The warning message is self-explanatory.

Issue 3

  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/plot/zonal_mean_xy_plot.py", line 130, in plot
    ax1.set_ylabel(da_test.long_name + " (" + da_test.units + ")")
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/common.py", line 278, in __getattr__
    raise AttributeError(
AttributeError: 'DataArray' object has no attribute 'long_name'

The long_name attribute looks like it is dropped at some point. Maybe when calculating zonal mean?

Issue 4

Traceback (most recent call last):
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 213, in _decode_cf_datetime_dtype
    result = decode_cf_datetime(example_value, units, calendar, use_cftime)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 321, in decode_cf_datetime
    dates = _decode_datetime_with_cftime(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 237, in _decode_datetime_with_cftime
    cftime.num2date(num_dates, units, calendar, only_use_cftime_datetimes=True)
  File "src/cftime/_cftime.pyx", line 580, in cftime._cftime.num2date
  File "src/cftime/_cftime.pyx", line 98, in cftime._cftime._dateparse
ValueError: 'months since' units only allowed for '360_day' calendar

Self-explanatory. The cftime library can't decode "months since" units with other calendar types. xc.open_dataset()/xc.open_mf_dataset() can decode non-CF time units. If one of these functions is used, maybe remove use_cftime=True to avoid using Xarray's cftime decoder.

Issue 5

Traceback (most recent call last):
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/conventions.py", line 432, in decode_cf_variables
    new_vars[k] = decode_cf_variable(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/conventions.py", line 283, in decode_cf_variable
    var = times.CFDatetimeCoder(use_cftime=use_cftime).decode(var, name=name)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 832, in decode
    dtype = _decode_cf_datetime_dtype(data, units, calendar, self.use_cftime)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 223, in _decode_cf_datetime_dtype
    raise ValueError(msg)
ValueError: unable to decode time units 'months since 1983-06' with 'the default calendar'. Try opening your dataset with decode_times=False or installing cftime if it is not installed.

Self-explanatory. The cftime library can't decode months since" units with other calendar types.xc.open_dataset()/xc.open_mf_dataset()can decode non-CF time units. If one of these functions is used, maybe removeuse_cftime=Trueto avoid using Xarray'scftime` decoder.

Issue 6

 File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/driver/utils/dataset_xr.py", line 254, in _get_global_attr_from_climo_dataset
    ds = xr.open_dataset(filepath)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/api.py", line 572, in open_dataset
    backend_ds = backend.open_dataset(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/netCDF4_.py", line 607, in open_dataset
    ds = store_entrypoint.open_dataset(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/store.py", line 58, in open_dataset
    ds = Dataset(vars, attrs=attrs)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/dataset.py", line 697, in __init__
    variables, coord_names, dims, indexes, _ = merge_data_and_coords(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/dataset.py", line 426, in merge_data_and_coords
    return merge_core(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/merge.py", line 724, in merge_core
    dims = calculate_dimensions(variables)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/variable.py", line 3007, in calculate_dimensions
    raise ValueError(
ValueError: dimension 'time' already exists as a scalar variable

Not 100% what is happening here when you open up the dataset.

Issue 7

  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/derivations/utils.py", line 175, in cosp_bin_sum
    prs: FileAxis = cld.getAxis(0)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/common.py", line 278, in __getattr__
    raise AttributeError(
AttributeError: 'DataArray' object has no attribute 'getAxis'

The cosp_bin_sum() and cosp_histogram_standardize() functions are refactored to use Xarray/xCDAT in this PR: https://github.com/E3SM-Project/e3sm_diags/pull/748/files#diff-2fd1bd98452189f43f530975ba12afc289337494f779cb5a2f97144d30820f22
We need to merge this PR first before this issue can be fixed.

Issue 8

  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/derivations/formulas.py", line 81, in pminuse_convert_units
    var.attrs["units"] == "kg/m2/s"
KeyError: 'units'

The units attribute looks like it is dropped at some point. Maybe when calculating zonal mean?

chengzhuzhang commented 7 months ago

Other than mypy complaints. There are still several problems causing some variables fails to generate plots. The log for a full run. The result can be found here

I listed the errors from the log below:

Issue 4

Traceback (most recent call last):
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 213, in _decode_cf_datetime_dtype
    result = decode_cf_datetime(example_value, units, calendar, use_cftime)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 321, in decode_cf_datetime
    dates = _decode_datetime_with_cftime(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 237, in _decode_datetime_with_cftime
    cftime.num2date(num_dates, units, calendar, only_use_cftime_datetimes=True)
  File "src/cftime/_cftime.pyx", line 580, in cftime._cftime.num2date
  File "src/cftime/_cftime.pyx", line 98, in cftime._cftime._dateparse
ValueError: 'months since' units only allowed for '360_day' calendar

* Self-explanatory. The `cftime` library can't decode `"months since"` units with other calendar types. `xc.open_dataset()`/`xc.open_mf_dataset()` can decode non-CF time units. If one of these functions is used, maybe remove `use_cftime=True` to avoid using  Xarray's `cftime` decoder.

Issue 5

Traceback (most recent call last):
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/conventions.py", line 432, in decode_cf_variables
    new_vars[k] = decode_cf_variable(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/conventions.py", line 283, in decode_cf_variable
    var = times.CFDatetimeCoder(use_cftime=use_cftime).decode(var, name=name)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 832, in decode
    dtype = _decode_cf_datetime_dtype(data, units, calendar, self.use_cftime)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/coding/times.py", line 223, in _decode_cf_datetime_dtype
    raise ValueError(msg)
ValueError: unable to decode time units 'months since 1983-06' with 'the default calendar'. Try opening your dataset with decode_times=False or installing cftime if it is not installed.

* Self-explanatory. The `cftime` library can't decode `months since" units with other calendar types. `xc.open_dataset()`/`xc.open_mf_dataset()`can decode non-CF time units. If one of these functions is used, maybe remove`use_cftime=True`to avoid using  Xarray's`cftime` decoder.

Issue 6

 File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/driver/utils/dataset_xr.py", line 254, in _get_global_attr_from_climo_dataset
    ds = xr.open_dataset(filepath)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/api.py", line 572, in open_dataset
    backend_ds = backend.open_dataset(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/netCDF4_.py", line 607, in open_dataset
    ds = store_entrypoint.open_dataset(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/store.py", line 58, in open_dataset
    ds = Dataset(vars, attrs=attrs)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/dataset.py", line 697, in __init__
    variables, coord_names, dims, indexes, _ = merge_data_and_coords(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/dataset.py", line 426, in merge_data_and_coords
    return merge_core(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/merge.py", line 724, in merge_core
    dims = calculate_dimensions(variables)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/variable.py", line 3007, in calculate_dimensions
    raise ValueError(
ValueError: dimension 'time' already exists as a scalar variable

* _Not 100% what is happening here when you open up the dataset._

Issue 7

  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/derivations/utils.py", line 175, in cosp_bin_sum
    prs: FileAxis = cld.getAxis(0)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/common.py", line 278, in __getattr__
    raise AttributeError(
AttributeError: 'DataArray' object has no attribute 'getAxis'

* The `cosp_bin_sum()` and `cosp_histogram_standardize()` functions are refactored to use Xarray/xCDAT in this PR: https://github.com/E3SM-Project/e3sm_diags/pull/748/files#diff-2fd1bd98452189f43f530975ba12afc289337494f779cb5a2f97144d30820f22

* We need to merge this PR first before this issue can be fixed.

Issue 8

  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/derivations/formulas.py", line 81, in pminuse_convert_units
    var.attrs["units"] == "kg/m2/s"
KeyError: 'units'

* The `units` attribute looks like it is dropped at some point. Maybe when calculating zonal mean?

Above are remaining issues. I think they are all data/variable related, including special dimensions, calendar and units: i.e. COSP variables and PminusE. Some could be fixed with @tomvothecoder 's PR. And I think I will revisit after merging.

chengzhuzhang commented 7 months ago

@tomvothecoder I think most codes for the feature is completed. Link to the viewer:https://portal.nersc.gov/cfs/e3sm/chengzhu/test_e3sm_refactor/zonal_mean_xy_model_to_obs_ANN/viewer/ I do realize new unit tests are needed. Could you please have a short demo/guide in the next Sprint? Thank you.

tomvothecoder commented 7 months ago

@tomvothecoder I think most codes for the feature is completed. Link to the viewer:portal.nersc.gov/cfs/e3sm/chengzhu/test_e3sm_refactor/zonal_mean_xy_model_to_obs_ANN/viewer

Awesome, thanks the update Jill.

I do realize new unit tests are needed. Could you please have a short demo/guide in the next Sprint? Thank you.

I believe all of the unit tests I've written are for the refactored utility modules (Dataset class, metrics functions, regridding functions, etc.). I don't have unit tests for the direct modules of the lat_lon_driver.py and lat_lon_plot.py yet (e.g., lat_lon_driver.run_diags()) since it will take a lot of time/work and we need to move fast. I'll think about how we can write unit tests for these complex modules in order to fit in time for it while we refactor.

chengzhuzhang commented 7 months ago

when reviewing the results, I found an issue that the units for 3d variables, and the title were not showing up correctly:

tomvothecoder commented 7 months ago

Issue 6

 File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/e3sm_diags/driver/utils/dataset_xr.py", line 254, in _get_global_attr_from_climo_dataset
    ds = xr.open_dataset(filepath)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/api.py", line 572, in open_dataset
    backend_ds = backend.open_dataset(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/netCDF4_.py", line 607, in open_dataset
    ds = store_entrypoint.open_dataset(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/backends/store.py", line 58, in open_dataset
    ds = Dataset(vars, attrs=attrs)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/dataset.py", line 697, in __init__
    variables, coord_names, dims, indexes, _ = merge_data_and_coords(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/dataset.py", line 426, in merge_data_and_coords
    return merge_core(
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/merge.py", line 724, in merge_core
    dims = calculate_dimensions(variables)
  File "/global/cfs/cdirs/e3sm/zhang40/conda_envs/e3sm_diags_dev_654_zonal_mean_xy/lib/python3.10/site-packages/xarray/core/variable.py", line 3007, in calculate_dimensions
    raise ValueError(
ValueError: dimension 'time' already exists as a scalar variable

Not 100% what is happening here when you open up the dataset.

I am running into this issue with dataset_xr.Dataset._get_global_attr_from_climo_dataset() in #748. The issue is the dataset incorrectly includes "time" as a scalar variable with a type as a single integer, rather than a 1D array. In this case, Xarray is correct to raise an error when trying to open the dataset (below quote).

This should actually raise an error at construction time for the DataArray. Variables with names matching dimensions are special in xarray: they are required to be 1D arrays with length matching the dimension size.

https://github.com/pydata/xarray/issues/1709

Example Code:

import xarray as xr
import cdms2

filepath = "/global/cfs/cdirs/e3sm/diagnostics/observations/Atm/climatology/MISRCOSP/MISRCOSP_ANN_climo.nc"

# ValueError: dimension 'time' already exists as a scalar variable
ds1 = xr.open_dataset(filepath)

# Works fine in cdms2 because it doesn't have the same check as Xarray
ds2 = cdms2.open(filepath)
print(ds2.variables)

"""
{'gw': <cdms2.fvariable.FileVariable at 0x7fad9618f7c0>,
 'CLDHGH_MISR': <cdms2.fvariable.FileVariable at 0x7fad95c8fa00>,
 'CLDINT_MISR': <cdms2.fvariable.FileVariable at 0x7fad95c8e2c0>,
 'CLDLOW_MISR': <cdms2.fvariable.FileVariable at 0x7fad95c8f7f0>,
 'CLDMED_MISR': <cdms2.fvariable.FileVariable at 0x7fad9602fa60>,
 'CLDTHICK_MISR': <cdms2.fvariable.FileVariable at 0x7fad9602edd0>,
 'CLDTHN_MISR': <cdms2.fvariable.FileVariable at 0x7fb1d94ee050>,
 'CLDTOT_MISR': <cdms2.fvariable.FileVariable at 0x7fb1d94ed930>,
 'CLMISR': <cdms2.fvariable.FileVariable at 0x7fb1d94edde0>,
 'misr_cth_bnds': <cdms2.fvariable.FileVariable at 0x7fb1d94ee2c0>,
 'misr_tau_bnds': <cdms2.fvariable.FileVariable at 0x7fb1d94edf30>,
 'time': <cdms2.fvariable.FileVariable at 0x7fb1d94ee0e0>}
"""

I'll check if writing a preprocess() function to convert "time" to coordinates is a possible workaround to this dataset issue (source).

tomvothecoder commented 7 months ago

I'll check if writing a preprocess() function to convert "time" to coordinates is a possible workaround to this dataset issue (source).

This doesn't work because it still opens the dataset first then applies the function to it.

drop_variables looks like it works. Is the "time" scalar variable ever used?

ds1 = xr.open_dataset(filepath, drop_variables="time")

drop_variables (str or iterable of str, optional) – A variable or list of variables to exclude from being parsed from the dataset. This may be useful to drop variables with problems or inconsistent values.

https://docs.xarray.dev/en/stable/generated/xarray.open_dataset.html

chengzhuzhang commented 7 months ago

This doesn't work because it still opens the dataset first then applies the function to it.

drop_variables looks like it works. Is the "time" scalar variable ever used

Good to know there is a workaround. I don't think the "time" scalar variable is ever used for this dataset. Though I don't have a good idea how to implement this dataset specific solution.

We can perhaps fix it in the source data?

tomvothecoder commented 7 months ago

This doesn't work because it still opens the dataset first then applies the function to it. drop_variables looks like it works. Is the "time" scalar variable ever used

Good to know there is a workaround. I don't think the "time" scalar variable is ever used for this dataset. Though I don't have a good idea how to implement this dataset specific solution.

We can perhaps fix it in the source data?

I just implemented a workaround in PR #748 here. Basically, if this specific issue ever pops up for a dataset then the "time" scalar variable is dropped and a new "time" scalar coordinate is added.

    def _open_climo_dataset(self, filepath: str) -> xr.Dataset:
        """Open a climatology dataset.

        Some climatology files have "time" as a scalar variable. If this
        variable is not a 1D array with a length matching the equivalent
        dimension size, Xarray will `raise ValueError: dimension 'time'
        already exists as a scalar variable`. We drop the "time" variable
        as a workaround to this issue for these cases and add new "time"
        coordinates.

        Parameters
        ----------
        filepath : str
            The path to a climatology dataset.

        Returns
        -------
        xr.Dataset
            The climatology dataset.

        Raises
        ------
        ValueError
            Raised for all ValueErrors other than "dimension 'time' already
            exists as a scalar variable".
        """
        args = {"path": filepath, "use_cftime": True, "add_bounds": ["X", "Y"]}

        try:
            ds = xc.open_dataset(**args)
        except ValueError as e:
            msg = str(e)

            if "dimension 'time' already exists as a scalar variable" in msg:
                ds = xc.open_dataset(**args, drop_variables=["time"])
                ds.coords["time"] = xr.DataArray(
                    name="time",
                    dims=["time"],
                    data=[0],
                    attrs={"axis": "T", "standard_name": "time"},
                )
            else:
                raise ValueError(msg)

        return ds

chengzhuzhang commented 7 months ago

This is a good solution!

chengzhuzhang commented 7 months ago

Ready for review. Issues 4, 5, 6, 7 from https://github.com/E3SM-Project/e3sm_diags/pull/752#issuecomment-1879174866 remain, but 6 and 7 are expected to be fixed in @tomvothecoder 's cosp histogram PR.

tomvothecoder commented 7 months ago

@chengzhuzhang I am close to done with PR #748. Let's merge that PR first, then get those fixes into your branch for this PR.

I'm also working on another regression testing notebook that compares .nc files. We can adapt that notebook for this branch for testing. The existing one only compares .json files, which some sets do not produce.

chengzhuzhang commented 7 months ago

@tomvothecoder Thanks for the heads up. Your plan sounds good!

chengzhuzhang commented 7 months ago

Based on the regression test result, several problems are found. They are grouped as follows:

Variables not generated and saved for NETCF, RESTOM and FLDSC, and all simulator related variables: e.g. CLDTOT_TAU1.3_ISCCP. error messsage: OSError: Variable 'NETCF' was not in the file '/global/cfs/cdirs/e3sm/e3sm_diags/postprocessed_e3sm_v2_data_for_e3sm_diags/20210528.v2rc3e.piControl.ne30pg2_EC30to60E2r2.chrysalis/climatology/rgr/20210528.v2rc3e.piControl.ne30pg2_EC30to60E2r2.chrysalis_ANN_005101_006012_climo.nc', nor was it defined in the derived variables dictionary.
OSError: Variable 'COSP_HISTOGRAM_MISR' was not in the file 'tests/integration/integration_test_data/CLD_MISR_20161118.beta0.FC5COSP.ne30_ne30.edison_ANN_climo.nc', nor was it defined in the derived variables dictionary. (CI/CD)
- Root cause: https://github.com/E3SM-Project/e3sm_diags/pull/752#discussion_r1478848688
- Solution: https://github.com/E3SM-Project/e3sm_diags/pull/752#discussion_r1478856532
Large difference between saved CERES-EBAF-TOA-v4.1 reference data comparison.
Shape mismatch for subset of ERA5, and MERRA2 reference data comparison. TMQ e.g. (shapes (180,), (361,) mismatch)

chengzhuzhang commented 7 months ago

For the 3rd problem in regression test https://github.com/E3SM-Project/e3sm_diags/pull/752#issuecomment-1927819504: It turned out when the reference data is not available or processed. (it (ds_ref will be the same dataset as ds_test) https://vscode.dev/github/E3SM-Project/e3sm_diags/blob/refactor/654-zonal_mean_xy/e3sm_diags/driver/zonal_mean_xy_driver.py#L136. This caused a dimension mismatch. We need to fix this assumption and allow only test data is being processed/saved and plotted.

Update1: Above is also the same cause for 3rd problem. ds_test is being saved when ds_ref is not available.

Update2: And why the reference datasets can't produced in development branch only is likely due to the the derived variable step...

chengzhuzhang commented 6 months ago

I followed the suggested change pointed out by https://github.com/E3SM-Project/e3sm_diags/pull/752#discussion_r1490045128. It fixed the CI/CD, but with remaining issues from regression tests, summarized here.

A different set of 21 variables are not created: FLUT, FLUTC, SOLIN, FLDS, FLNS, FLNSC, FSDS, FLDSC, FSNS, FSNSC,ISCCPCOSP-CLDTOT_TAU1.3_9.4_ISCCP, ISCCPCOSP-CLDTOT_TAU1.3_ISCCP, MODISCOSP-CLDHGH/TOT_TAU1.3_9.4_MODIS, MODISCOSP-CLDHGH/TOT_TAU1.3_MODIS, ERA5-TMQ, MERRA2-TMQ
Variables diff between refactored vs main: 645_unmatched.txt

@tomvothecoder I'm documenting the remaining issues. I think these will recur in lat_lon and polar testing. We can merge this branch for now and try to fix in another PR. Could you please help merging?

tomvothecoder commented 6 months ago

I followed the suggested change pointed out by #752 (comment). It fixed the CI/CD, but with remaining issues from regression tests, summarized here.
1. A different set of 21 variables are not created: `FLUT`, `FLUTC`, `SOLIN`, `FLDS`, `FLNS`, `FLNSC`, `FSDS`, `FLDSC`, `FSNS`, `FSNSC`,`ISCCPCOSP-CLDTOT_TAU1.3_9.4_ISCCP`, `ISCCPCOSP-CLDTOT_TAU1.3_ISCCP`, `MODISCOSP-CLDHGH/TOT_TAU1.3_9.4_MODIS`, `MODISCOSP-CLDHGH/TOT_TAU1.3_MODIS`, `ERA5-TMQ`, `MERRA2-TMQ`

2. Variables diff between refactored vs main:
   [645_unmatched.txt](https://github.com/E3SM-Project/e3sm_diags/files/14289669/645_unmatched.txt)
@tomvothecoder I'm documenting the remaining issues. I think these will recur in lat_lon and polar testing. We can merge this branch for now and try to fix in another PR. Could you please help merging?

Got it, we can transfer these to another GitHub issue.

tomvothecoder commented 6 months ago

Actually, I'll take a brief look at these issues and try to debug while I'm at it.

chengzhuzhang commented 6 months ago

Actually, I'll take a brief look at these issues and try to debug while I'm at it.

Thank you!

chengzhuzhang commented 6 months ago

@tomvothecoder Thank you for the perseverance to fix the derived variable logic and those COSP variable calculation!! The PR is in a much better shape after the fix!

E3SM-Project / e3sm_diags