pySTEPS / pysteps

Python framework for short-term ensemble prediction systems.
https://pysteps.github.io/
BSD 3-Clause "New" or "Revised" License
466 stars 168 forks source link

BUG: Fix T-DaTing labels when continuing tracking #280

Closed ritvje closed 2 years ago

ritvje commented 2 years ago

Hi! We came across this bug when trying to use the tracking functions. When continuing the tracking from previously tracked list, the tracks were weird, for example when tracking 3 x 20-minute intervals:

Track

This is because when setting the unique IDs for the tracks, the IDs of the previous cells are not considered. This PR fixes that. Here are the tracks with this fix:

Track_fixed

dnerini commented 2 years ago

Very good, thanks a lot @ritvje for the contribution!

@feldmann-m do you want to have a look at this PR?

codecov[bot] commented 2 years ago

Codecov Report

Merging #280 (608f8d8) into master (464525f) will increase coverage by 0.06%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   82.32%   82.39%   +0.06%     
==========================================
  Files         158      158              
  Lines       12130    12165      +35     
==========================================
+ Hits         9986    10023      +37     
+ Misses       2144     2142       -2     
Flag Coverage Δ
unit_tests 82.39% <100.00%> (+0.06%) :arrow_up:

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

Impacted Files Coverage Δ
pysteps/tests/test_tracking_tdating.py 100.00% <100.00%> (ø)
pysteps/tracking/tdating.py 91.47% <100.00%> (+1.70%) :arrow_up:
pysteps/tests/test_blending_skill_scores.py 100.00% <0.00%> (ø)
pysteps/tests/test_blending_steps.py 99.13% <0.00%> (+0.01%) :arrow_up:
pysteps/blending/steps.py 83.91% <0.00%> (+0.18%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 464525f...608f8d8. Read the comment docs.

ritvje commented 2 years ago

Sure, I can try to write the test! There are some clear indicators of the bug in my data, like that the track list is shorter at the second time interval than first and some tracks are not continuous in time, but I'll have to see if those repeat in the test data.

ritvje commented 2 years ago

I now added a test. I tried to test that the logic in the result tracks is correct. However, even without the fix it won't fail for all datasets (for example, the first timesteps of FMI test data), most likely if there are no unmatched cells in the second interval. Which makes sense, since the bug in setting the ID only affects unmatched cells. But it should fail for this configuration without the fix and pass with it.

Also, when I tried to run the tests with tox (as instructed in the developer guide), it seems that the test (and also the other tdating test) is skipped because skimage is not installed in the environment. Is this fine or should I add skimage there somehow?

feldmann-m commented 2 years ago

Thank you @ritvje for raising this issue! The fix looks great to me, this should enable the tracking module to continue tracking from existing lists.