Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test outlier detection #298

Merged
merged 3 commits into from
Sep 22, 2022
Merged

Conversation

chiara-arpae
Copy link
Contributor

@chiara-arpae chiara-arpae commented Aug 28, 2022

[ERAD]Test that input arrays with more than 2 dimensions raise a ValueError exception](https://app.codecov.io/gh/pySTEPS/pysteps/compare/283/tree/pysteps/utils/cleansing.py#D1L173)

Copy link
Contributor

@katelbach katelbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot for your contribution, i proposed a few modifications to your code

pysteps/tests/test_utils_cleansing.py Outdated Show resolved Hide resolved
@@ -174,3 +174,9 @@ def test_detect_outlier_multivariate_local():
V[-1] = (-3, 3)
outliers = cleansing.detect_outliers(V, 4, coord=X, k=50)
assert outliers.sum() == 2

def test_detect_outlier_input_dims():
V = np.zeros((20, 3, 2))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you test a list of input dimensions/different cases?

Copy link
Member

@aperezhortal aperezhortal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chiara-arpae for the contribution.

The test is failing because there is a bug in the detect_outliers function.
When the dimensions are incorrect, the code raising the exception has a error:

raise ValueError(
    f"input_array must have 1 (n) or 2 dimensions (n, m), "
    f"but it has {coord.ndim}"
)

Here, {coord.ndim} should be {input_array.ndim}

Do you want to add the fix to bug?

@dnerini
Copy link
Member

dnerini commented Sep 16, 2022

hey @chiara-arpae please let us know if something is not clear in the comments left above. The pull request is almost ready to be merged, just few final changes are needed ;)

@chiara-arpae
Copy link
Contributor Author

Hi! I'm very sorry for late. I should have committed the suggested correction few minutes ago. Please let me know if it's all fine.

correcting input_array instead of coords in log message
@chiara-arpae
Copy link
Contributor Author

chiara-arpae commented Sep 16, 2022 via email

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #298 (dd3685d) into master (94a11b3) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
+ Coverage   82.71%   82.74%   +0.03%     
==========================================
  Files         160      160              
  Lines       12292    12310      +18     
==========================================
+ Hits        10167    10186      +19     
+ Misses       2125     2124       -1     
Flag Coverage Δ
unit_tests 82.74% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
pysteps/utils/cleansing.py 82.82% <ø> (+1.01%) ⬆️
pysteps/tests/test_utils_cleansing.py 100.00% <100.00%> (ø)
pysteps/feature/tstorm.py 94.40% <0.00%> (ø)
pysteps/tracking/tdating.py 91.47% <0.00%> (ø)
...teps/tests/test_nowcasts_lagrangian_probability.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

removed k parameter from cleansing.detect_outliers in test.test_utils_cleansing.test_detect_outlier_input_dims() because not necessary for test and coords is not passed.
@dnerini
Copy link
Member

dnerini commented Sep 22, 2022

hey @chiara-arpae no worries and thanks for getting back on this! It looks good to go! :-)

@dnerini dnerini merged commit cb79b47 into pySTEPS:master Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants