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

Fix combine_metadata not handling lists of different sizes #1643

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Apr 17, 2021

The combine_metadata utility function doesn't seem to handle lists of different sizes. Before this PR you'd get something like this:

~/repos/git/satpy/satpy/dataset/metadata.py in _all_values_equal(values)
    179 def _all_values_equal(values):
    180     try:
--> 181         return _pairwise_all(nan_allclose, values)
    182     except TypeError:
    183         return _pairwise_all(eq, values)

~/repos/git/satpy/satpy/dataset/metadata.py in _pairwise_all(func, values)
    144 def _pairwise_all(func, values):
    145     for value in values[1:]:
--> 146         if not func(values[0], value):
    147             return False
    148     return True

<__array_function__ internals> in allclose(*args, **kwargs)

~/miniconda3/envs/satpy_py37/lib/python3.7/site-packages/numpy/core/numeric.py in allclose(a, b, rtol, atol, equal_nan)
   2254 
   2255     """
-> 2256     res = all(isclose(a, b, rtol=rtol, atol=atol, equal_nan=equal_nan))
   2257     return bool(res)
   2258 

<__array_function__ internals> in isclose(*args, **kwargs)

~/miniconda3/envs/satpy_py37/lib/python3.7/site-packages/numpy/core/numeric.py in isclose(a, b, rtol, atol, equal_nan)
   2363     yfin = isfinite(y)
   2364     if all(xfin) and all(yfin):
-> 2365         return within_tol(x, y, atol, rtol)
   2366     else:
   2367         finite = xfin & yfin

~/miniconda3/envs/satpy_py37/lib/python3.7/site-packages/numpy/core/numeric.py in within_tol(x, y, atol, rtol)
   2344     def within_tol(x, y, atol, rtol):
   2345         with errstate(invalid='ignore'):
-> 2346             return less_equal(abs(x-y), atol + rtol * abs(y))
   2347 
   2348     x = asanyarray(a)

ValueError: operands could not be broadcast together with shapes (4,) (0,) 

This PR fixes this by catching this ValueError when trying to compare lists. I wanted to have a simple additional check like if len(x) != len(y): return False, but the function that is triggering this issue is meant to compare any value including those that may not have a len parameter.

@codecov
Copy link

codecov bot commented Apr 17, 2021

Codecov Report

Merging #1643 (c3e6f0c) into master (7aa706b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1643   +/-   ##
=======================================
  Coverage   92.62%   92.62%           
=======================================
  Files         258      258           
  Lines       37810    37828   +18     
=======================================
+ Hits        35022    35040   +18     
  Misses       2788     2788           
Flag Coverage Δ
behaviourtests 4.81% <0.00%> (-0.01%) ⬇️
unittests 92.76% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
satpy/dataset/metadata.py 98.66% <100.00%> (ø)
satpy/tests/test_dataset.py 100.00% <100.00%> (ø)
satpy/readers/electrol_hrit.py 91.66% <0.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 7aa706b...c3e6f0c. Read the comment docs.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple styling/testing comments.

satpy/tests/test_dataset.py Show resolved Hide resolved
@djhoese djhoese requested a review from mraspaud April 20, 2021 15:26
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud mraspaud merged commit f9a8f8d into pytroll:master Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't resample GOES Meso data when using night IR composite
2 participants