Closed pulkkins closed 2 years ago
Merging #290 (2b29e84) into master (49cc4f5) will increase coverage by
0.00%
. The diff coverage is66.66%
.
@@ Coverage Diff @@
## master #290 +/- ##
=======================================
Coverage 82.60% 82.60%
=======================================
Files 159 159
Lines 12190 12196 +6
=======================================
+ Hits 10069 10074 +5
- Misses 2121 2122 +1
Flag | Coverage Δ | |
---|---|---|
unit_tests | 82.60% <66.66%> (+<0.01%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
pysteps/extrapolation/semilagrangian.py | 70.10% <0.00%> (-0.31%) |
:arrow_down: |
pysteps/nowcasts/linda.py | 91.72% <50.00%> (-0.14%) |
:arrow_down: |
pysteps/nowcasts/anvil.py | 69.73% <100.00%> (+0.23%) |
:arrow_up: |
pysteps/nowcasts/extrapolation.py | 74.19% <100.00%> (+1.77%) |
:arrow_up: |
pysteps/nowcasts/sprog.py | 90.30% <100.00%> (ø) |
|
pysteps/nowcasts/sseps.py | 87.41% <100.00%> (+0.02%) |
:arrow_up: |
pysteps/nowcasts/steps.py | 86.42% <100.00%> (ø) |
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 49cc4f5...2b29e84. Read the comment docs.
Looks all good to me! Can you just confirm my impression that this PR doesn't introduce any breaking change?
I can confirm that. I also tested all the currently implemented nowcasting methods using input data with and without NaN values. This is not included in the automatic tests, which raises a question. Should we extend the test coverage (although it might significantly increase the run time of the tests)?
I also tested all the currently implemented nowcasting methods using input data with and without NaN values. This is not included in the automatic tests, which raises a question. Should we extend the test coverage (although it might significantly increase the run time of the tests)?
It’s a fair point, I’d generally go for more test coverage. If we have an issue with computation time, I think we could fairly easily make them more efficient by reducing the size of the test data. Should we take this into a separate issue or would fix that in here?
Looks all good to me! Can you just confirm my impression that this PR doesn't introduce any breaking change?
I can confirm that. I also tested all the currently implemented nowcasting methods using input data with and without NaN values. This is not included in the automatic tests, which raises a question. Should we extend the test coverage (although it might significantly increase the run time of the tests)?
It’s a fair point, I’d generally go for more test coverage. If we have an issue with computation time, I think we could fairly easily make them more efficient by reducing the size of the test data. Should we take this into a separate issue or would fix that in here?
I would take that into a separate issue. It seems that there are quite many gaps in the test coverage of the nowcasting methods.
This pull requests fixes inconsistencies and bugs related to handling of NaN values in the semi-Lagrangian extrapolation: