Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not expand_dims if dim is already present #324

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cosunae
Copy link

@cosunae cosunae commented Dec 10, 2022

Description

while loading a dataset from a grib file that contains both grib2 and grib1 messages, most of the variables look like this:

<xarray.DataArray 'sd' (latitude: 301, longitude: 571)>
array([[0.        , 0.        , 0.        , ..., 0.        , 0.        ,
        0.00024414],
       [0.        , 0.        , 0.        , ..., 0.        , 0.        ,
        0.        ],
       [0.        , 0.        , 0.        , ..., 0.        , 0.        ,
        0.        ],
       ...,
       [0.        , 0.        , 0.        , ..., 0.        , 0.        ,
        0.        ],
       [0.        , 0.        , 0.        , ..., 0.        , 0.        ,
        0.        ],
       [0.        , 0.        , 0.        , ..., 0.        , 0.        ,
        0.        ]], dtype=float32)
Coordinates:
    time       datetime64[ns] 2022-10-26
    step       timedelta64[ns] 03:00:00
    surface    float64 0.0
  * latitude   (latitude) float64 65.0 64.9 64.8 64.7 ... 35.3 35.2 35.1 35.0
  * longitude  (longitude) float64 -10.0 -9.9 -9.8 -9.7 ... 46.7 46.8 46.9 47.0
Attributes: (12/35)
    GRIB_paramId:                             500044
    GRIB_dataType:                            fc
    GRIB_numberOfPoints:                      171871
    GRIB_typeOfLevel:                         surface
    GRIB_stepUnits:                           1
    GRIB_stepType:                            instant
    ...                                       ...
    GRIB_centre:                              78
    GRIB_subCentre:                           0
    GRIB_scanningMode:                        0
    GRIB_typeOfGeneratingProcess:             2
    GRIB_generatingProcessIdentifier:         153
    GRIB_bitsPerValue:                        16

There are two dimension coordinates (latitude, longitude) and few other non-dimension coordinates.
However some data arrays additionally made step a dimension coordinate :

<xarray.DataArray (step: 1, latitude: 301, longitude: 571)>
array([[[1.83900197e-01, 1.22149785e-01, 4.00543213e-02, ...,
         9.69568888e-03, 7.23203023e-03, 5.08626302e-03],
        [9.37779744e-03, 5.96046448e-03, 5.56310018e-04, ...,
         6.75519307e-03, 7.07308451e-03, 4.68889872e-03],
        [0.00000000e+00, 0.00000000e+00, 0.00000000e+00, ...,
         3.17891439e-04, 3.17891439e-04, 3.17891439e-04],
        ...,
        [3.23454539e-02, 8.02675883e-03, 0.00000000e+00, ...,
         0.00000000e+00, 0.00000000e+00, 0.00000000e+00],
        [1.58945719e-04, 0.00000000e+00, 0.00000000e+00, ...,
         0.00000000e+00, 0.00000000e+00, 0.00000000e+00],
        [0.00000000e+00, 0.00000000e+00, 0.00000000e+00, ...,
         0.00000000e+00, 0.00000000e+00, 0.00000000e+00]]])
Coordinates:
    time       datetime64[ns] 2022-10-26
  * step       (step) timedelta64[ns] 03:00:00
    surface    float64 0.0
  * latitude   (latitude) float64 65.0 64.9 64.8 64.7 ... 35.3 35.2 35.1 35.0
  * longitude  (longitude) float64 -10.0 -9.9 -9.8 -9.7 ... 46.7 46.8 46.9 47.0
Attributes: (12/35)
    GRIB_paramId:                             500043
    GRIB_dataType:                            fc
    GRIB_numberOfPoints:                      171871
    GRIB_typeOfLevel:                         surface
    GRIB_stepUnits:                           1
    GRIB_stepType:                            instant
    ...                                       ...
    GRIB_centre:                              78
    GRIB_subCentre:                           0
    GRIB_scanningMode:                        0
    GRIB_typeOfGeneratingProcess:             2
    GRIB_generatingProcessIdentifier:         153
    GRIB_bitsPerValue:                        16

I did not investigate further why this difference in behaviour, since anyhow I thought the dataset is in a valid state and both representations are fine.

However when trying to write back to grib, using canonical_dataarray_to_grib, cfgrib will raise an exception.
Here

coords_names, data_var = expand_dims(data_var)

the function tries to add make all coordinates a dimension coordinate, i.e. adding a dimension with the same name and size 1.
However, that method fails (raises) to add dimension "step" in the data array above since it already exists.

Below a stack trace (based on cfgrib==0.9.10.3)

  File "run_flexpart.py", line 111, in write_to_grib
    cfgrib.xarray_to_grib.canonical_dataarray_to_grib(
  File "/scratch/cosuna/miniconda3/envs/idpi_test/lib/python3.10/site-packages/cfgrib/xarray_to_grib.py", line 221, in canonical_dataarray_to_grib
    coords_names, data_var = expand_dims(data_var)
  File "/scratch/cosuna/miniconda3/envs/idpi_test/lib/python3.10/site-packages/cfgrib/xarray_to_grib.py", line 171, in expand_dims
    data_var = data_var.expand_dims(coord_name)
  File "/scratch/cosuna/miniconda3/envs/idpi_test/lib/python3.10/site-packages/xarray/core/dataarray.py", line 2511, in expand_dims
    ds = self._to_temp_dataset().expand_dims(dim, axis)
  File "/scratch/cosuna/miniconda3/envs/idpi_test/lib/python3.10/site-packages/xarray/core/dataset.py", line 3913, in expand_dims
    raise ValueError(f"Dimension {d} already exists.")
ValueError: Dimension step already exists.

@FussyDuck
Copy link

FussyDuck commented Dec 10, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ iainrussell
❌ cosunae


cosunae seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@iainrussell
Copy link
Member

Hello @cosunae, many thanks for your contribution! Could you just give a little more concrete detail about when this problem manifests? And would you also be able to add a small test to the test suite to confirm that the fix works please?
Cheers,
Iain

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (00e936e) 95.65% compared to head (f1c8fb3) 95.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
+ Coverage   95.65%   95.67%   +0.01%     
==========================================
  Files          26       26              
  Lines        2073     2082       +9     
  Branches      238      238              
==========================================
+ Hits         1983     1992       +9     
  Misses         59       59              
  Partials       31       31              
Impacted Files Coverage Δ
cfgrib/xarray_to_grib.py 86.36% <100.00%> (ø)
tests/test_40_xarray_to_grib_regular_ll.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cosunae
Copy link
Author

cosunae commented Jan 4, 2023

Hi @iainrussell , thanks for looking into it and apologies for the delay and the original lack of details.
I tried to provide a more detailed explanation in the description and added a small test that raises without the fix and passes with the fix. The test simply tries to write a dataarray with a step coordinate and dimension of size 1. But I am not familiar with how the tests are organized in cfgrib, so let me know if you would like to have it somewhere else.

@cosunae
Copy link
Author

cosunae commented Jan 4, 2023

btw, I did sign the cla, not sure why it does not recognize it.
Also are there any CI tests running on the PR ? It seems that the appveyor is only building (at least I can not find the tests)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants