openradar / xradar

A tool to work in weather radar data in xarray
https://docs.openradarscience.org/projects/xradar
MIT License
85 stars 17 forks source link

FIX ipol_time not working if last azimuth is first #114

Closed egouden closed 1 year ago

egouden commented 1 year ago

The function util.ipol_time is not working if the last azimuth is the first timestamp. This causes a zero-length array. You might want to see the change itself for more insight.

codecov[bot] commented 1 year ago

Codecov Report

Merging #114 (c5f23df) into main (6fe2ea0) will decrease coverage by 0.03%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #114      +/-   ##
==========================================
- Coverage   88.08%   88.05%   -0.03%     
==========================================
  Files          19       19              
  Lines        3297     3298       +1     
==========================================
  Hits         2904     2904              
- Misses        393      394       +1     
Flag Coverage Δ
unittests 88.05% <100.00%> (-0.03%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
xradar/util.py 96.52% <100.00%> (+0.03%) :arrow_up:

... and 1 file with indirect coverage changes

kmuehlbauer commented 1 year ago

Thanks @egouden. Can you add a test for this?

egouden commented 1 year ago

The diff coverage is 100%. It is therefore unclear to me what you would like to test.

kmuehlbauer commented 1 year ago

A test which fails with current code and works with your fix.

There is a test function here :

https://github.com/openradar/xradar/blob/main/tests/test_util.py#L84

It would be great, if you could add a similar tests, even stripped down using no IO and just ipol_time with synthetic data with that special condition which raises this bug.

I know we are undertesting in many locations and it would be great to gradually enhance test coverage bit by bit.

The current test coverage just tells us that it runs with the current test Radar data. But we might miss certain issues, which are not raised by those files.

Thanks.

kmuehlbauer commented 1 year ago

Superseded by #117. Thanks @egouden for getting this into traction.