Closed EsmailGhaemi closed 2 years ago
Merging #248 (193900c) into master (bd94785) will increase coverage by
0.14%
. The diff coverage is92.02%
.
@@ Coverage Diff @@
## master #248 +/- ##
==========================================
+ Coverage 80.78% 80.92% +0.14%
==========================================
Files 140 142 +2
Lines 10625 10759 +134
==========================================
+ Hits 8583 8707 +124
- Misses 2042 2052 +10
Flag | Coverage Δ | |
---|---|---|
unit_tests | 80.92% <92.02%> (+0.14%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
pysteps/tests/test_postprocessing_ensemblestats.py | 100.00% <ø> (ø) |
|
pysteps/tests/test_verification_detcatscores.py | 100.00% <ø> (ø) |
|
pysteps/tests/test_verification_detcontscores.py | 100.00% <ø> (ø) |
|
pysteps/tests/test_verification_probscores.py | 100.00% <ø> (ø) |
|
pysteps/tests/test_verification_spatialscores.py | 100.00% <ø> (ø) |
|
pysteps/verification/interface.py | 46.80% <50.00%> (-0.92%) |
:arrow_down: |
pysteps/verification/salscores.py | 90.42% <90.42%> (ø) |
|
pysteps/tests/helpers.py | 94.28% <100.00%> (ø) |
|
pysteps/tests/test_verification_salscores.py | 100.00% <100.00%> (ø) |
|
pysteps/verification/spatialscores.py | 85.42% <100.00%> (+0.07%) |
:arrow_up: |
... and 3 more |
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 bd94785...193900c. Read the comment docs.
Thanks for the PR @EsmailGhaemi !
I took the liberty to advance with the more obvious changes, including the suggestions from @aperezhortal. If possible, I'd like to ask @feldmann-m for a review, since this implementation now heavily relies on her feature detection method.
Next, I'll work on some more refactoring, including the use of the _init, _accum, and _compute methods.
To maintain consistency across verification metric interfaces, perhaps the auxiliary functions used for the SAL computation can be moved to a separate module (like
sal_utils.py
orsal.py
). And implementsal
,sal_init
,sal_accum
,sal_merge
, andsal_compute
in thespatialscores.py
module. By doing so the SAL internals using pandas dataframes and the pysteps.verification function using arrays and dictionaries are kept separated.
I also thought about it, and at the moment I'm more of the idea that SAL can be considered a separate case as init, accum, merge, and compute do not really apply here... thus commit 71033e4 extract all the SAL methods into a separate module. What do you think?
I also thought about it, and at the moment I'm more of the idea that SAL can be considered a separate case as init, accum, merge, and compute do not really apply here... thus commit 71033e4 extract all the SAL methods into a separate module. What do you think?
:+1:
Also, maybe we can import the sal.sal()
function the verification metrics to make it accessible from the spatialscores
.
from pysteps.verification import spatialscores
spatialscores.sal(array1, array2)
In this way, all the spatial metrics can be found in the same place.
Tests for python=3.6 are still failing...
I have troubles reproducing the error locally (still working on it).
Also, #253 might solve the problem by setting the minimum python version to 3.7 ...
Setting the minimum python version to 3.7 (d92904a) fixed the remaining failing tests.
I'm merging this PR, thanks again to @EsmailGhaemi for the original contribution and to @aperezhortal and @feldmann-m for the valuable inputs during the review!
For plotting: I think using a scatter plot with the L component as the color arg would work. The input can be sal or accum_sal. I'll add it ASAP.
@EsmailGhaemi it's an excellent idea to provide such a plotting functionality! I suggest to introduce it with a separate PR, perhaps together with an example script for the gallery?
The Structure-Amplitude-Location (SAL) method has been added. Note that the original approach suggested by (Wernli et al., 2009) is based on a simple minimum threshold (f), but here we used tstorm function to detect objects (Feldmann et al., 2021). Using this function allows to have a more realistic way to detect precipitation as objects.