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 delayed generation of composites and composite resolution #828

Merged
merged 8 commits into from Jun 27, 2019

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Jun 24, 2019

This PR fixes three bugs in the composite generation part of satpy.

  1. The empty_node is now a class attribute to make sure it is the same across multiple instances of the dependency tree. This fixes a bug appearing when the composite generation is delayed, which causes the dependency tree to be copied and previously a net empty_node to be created.
  2. The resolution of a composite made of datasets with the same resolution is now set (it was None).
  3. Two composites with the same dependencies, loaded one after the other with different resolutions are now loaded with the proper respective resolution
  • Tests added and test suite added to parent suite
  • Tests passed
  • Passes flake8 satpy

@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

Merging #828 into master will increase coverage by 0.04%.
The diff coverage is 97.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #828      +/-   ##
==========================================
+ Coverage    83.9%   83.95%   +0.04%     
==========================================
  Files         167      167              
  Lines       24445    24512      +67     
==========================================
+ Hits        20511    20578      +67     
  Misses       3934     3934
Impacted Files Coverage Δ
satpy/tests/utils.py 97.08% <100%> (ø) ⬆️
satpy/tests/compositor_tests/__init__.py 99.53% <100%> (ø) ⬆️
satpy/node.py 94.79% <100%> (+0.28%) ⬆️
satpy/tests/test_scene.py 99.48% <100%> (+0.01%) ⬆️
satpy/composites/__init__.py 74.74% <60%> (+0.09%) ⬆️

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 2654cf3...3301cf2. Read the comment docs.

@coveralls
Copy link

coveralls commented Jun 24, 2019

Coverage Status

Coverage increased (+0.04%) to 83.926% when pulling 3301cf2 on mraspaud:fix-modis-processing into 2654cf3 on pytroll:master.

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.

LGTM. Would it be possible to add some tests for this?

@mraspaud
Copy link
Member Author

Good question :)

@mraspaud
Copy link
Member Author

Test added

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.

How hard would it be to add a test for the delayed generation "empty" node stuff? I suppose that would be with the tests I usually write for reading. We must have one for delayed generation already.

@mraspaud
Copy link
Member Author

I think this is ready for re-review. I'd like to have it included in the patch release if possible.

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.

There are a couple things that could be cleaned up, but otherwise it looks good to me. I had one question about one of the tests.

Nice job deciphering my Scene/reader tests and adding your own.

satpy/node.py Outdated Show resolved Hide resolved
satpy/tests/test_scene.py Outdated Show resolved Hide resolved
satpy/tests/test_scene.py Show resolved Hide resolved
@djhoese
Copy link
Member

djhoese commented Jun 27, 2019

Travis notification is stuck, but all tests pass. Merging now.

@djhoese djhoese merged commit b731bf1 into pytroll:master Jun 27, 2019
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