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

Refactor the combine_metadata function and allow numpy arrays to be combined #1309

Merged
merged 9 commits into from Aug 11, 2020

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Aug 10, 2020

The valid_range array was not being combined properly because each dataarray had it's own copy of it. This refactor changes the behaviour of combine_metadata to just check the identities of lazy arrays, and use np.allclose otherwise.

  • Tests added
  • Tests passed
  • Passes flake8 satpy

…ombined

The `valid_range` array was not being combined properly because each dataarray had it's own copy of it. This refactor changes the behaviour of `combine_metadata` to just check the identities of lazy arrays, and use np.allclose otherwise.
@mraspaud mraspaud self-assigned this Aug 10, 2020
@mraspaud mraspaud changed the title Refactor the combine_metadata function and allow numpy arrays to be c… Refactor the combine_metadata function and allow numpy arrays to be combined Aug 10, 2020
@mraspaud mraspaud marked this pull request as ready for review August 10, 2020 15:52
@mraspaud mraspaud requested a review from djhoese as a code owner August 10, 2020 15:52
@coveralls
Copy link

coveralls commented Aug 10, 2020

Coverage Status

Coverage increased (+0.02%) to 90.037% when pulling 1823109 on mraspaud:fix-np-scalar-combine into 4924cba on pytroll:master.

@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

Merging #1309 into master will increase coverage by 0.01%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1309      +/-   ##
==========================================
+ Coverage   90.01%   90.03%   +0.01%     
==========================================
  Files         220      220              
  Lines       32254    32309      +55     
==========================================
+ Hits        29035    29090      +55     
  Misses       3219     3219              
Impacted Files Coverage Δ
satpy/dataset.py 92.93% <94.50%> (+0.32%) ⬆️
satpy/tests/test_dataset.py 100.00% <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 fd7f313...1823109. Read the comment docs.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

I think this looks good, but honestly it is hard to review with this new coding style. I know the code is overall more organized but I have to jump around between so many functions...this looks fine. I think originally Gerrit and I were trying to avoid every comparing numpy arrays. Not sure if that was how it was actually implemented before, but I'm fine with what you've done here.

@mraspaud
Copy link
Member Author

I go from the assumption that heavy arrays will be lazy, so pure numpy arrays are OK to compare. As you can see in the tests, this was to compare eg valid_range 2-element arrays. Another solution would be to set a hardcoded limit on the length of arrays that we are OK with comparing.

@djhoese
Copy link
Member

djhoese commented Aug 11, 2020

Another solution would be to set a hardcoded limit on the length of arrays that we are OK with comparing.

Might be a good idea in the future. I wish xarray would return valid_range as a tuple/list instead of an array. It will always be two elements.

@mraspaud
Copy link
Member Author

Regarding jumping between functions: I tried to always put a function's definition after it was used, so that we can go from high level to low level by reading from top to bottom. But yeah, reviewing isn't probably as linear anymore, but hopefully the intent and implementation of the functions is clearer now.

@mraspaud mraspaud merged commit cdc8ce5 into pytroll:master Aug 11, 2020
@mraspaud mraspaud deleted the fix-np-scalar-combine branch August 12, 2020 08:39
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.

None yet

3 participants