search

ecmwf / cfgrib

A Python interface to map GRIB files to the NetCDF Common Data Model following the CF Convention using ecCodes
Apache License 2.0
407 stars 77 forks source link

Prevent merging of different variables with the same name and date but different attributes #339

Closed Metamess closed 1 year ago

Metamess commented 1 year ago

closes #336

Fixes a bug where cfgrib would incorrectly combine two different variables if they happened to have the same shortName and data. For example, all-zeroes data GRIB messages with stepType='instant' and stepType='avg' attributes for csrwe (Convective snowfall rate water equivalent) on days without snow.

FussyDuck commented 1 year ago

CLA assistant check
All committers have signed the CLA.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (3951355) 95.44% compared to head (597b2ff) 95.44%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #339 +/- ## ======================================= Coverage 95.44% 95.44% ======================================= Files 26 26 Lines 2040 2040 Branches 236 236 ======================================= Hits 1947 1947 Misses 62 62 Partials 31 31 ``` | [Impacted Files](https://app.codecov.io/gh/ecmwf/cfgrib/pull/339?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ecmwf) | Coverage Δ | | |---|---|---| | [cfgrib/xarray\_store.py](https://app.codecov.io/gh/ecmwf/cfgrib/pull/339?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ecmwf#diff-Y2ZncmliL3hhcnJheV9zdG9yZS5weQ==) | `93.84% <100.00%> (ø)` | |

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

iainrussell commented 1 year ago

Hi @Metamess,

Thank you for providing this great fix! I think the xarray documentation is not great when explaining exactly how all this matching works, but I could get the relevant data, reproduce the bug and confirm that your fix works. I just added a couple of tests using very small data, and fixed the 'black' formatting (you may have noticed that the 'code style' test failed. Hopefully it will go through now, and then I will merge it in. I also appreciated your comprehensive description of the problem - very helpful! Cheers, Iain