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

Update add_overlay to make use of the full pycoast capabilities #934

Merged
merged 11 commits into from
Oct 25, 2019

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Oct 11, 2019

This requires pytroll/pycoast#32 and pytroll/trollimage#57 to be merged and released

  • Tests added and test suite added to parent suite
  • Tests passed
  • Passes flake8 satpy
  • Fully documented

@mraspaud mraspaud added enhancement code enhancements, features, improvements component:writers labels Oct 11, 2019
@mraspaud mraspaud self-assigned this Oct 11, 2019
@mraspaud
Copy link
Member Author

After thinking more about this, I think trollimage.XRImage should expose a method called something like pil_apply that allows the user to apply some pillow operations on the image. This way, satpy wouldn't need to make any assumption on how an XRImage works, so we could call for example

new_img = orig_img.pil_apply(_burn_overlays)(*args, **kwargs)

(_burn_overlays would have to be modify of course)
The delaying of the pil operations would then be done in trollimage.

@mraspaud mraspaud marked this pull request as ready for review October 15, 2019 22:28
@mraspaud mraspaud requested a review from djhoese as a code owner October 15, 2019 22:28
if overlays is None:
# fill with sensible defaults
general_params = {'outline': color or (0, 0, 0),
'width': width or 0.5}
Copy link
Member

Choose a reason for hiding this comment

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

Why are these sensible defaults not the defaults in pycoast?

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha good question :) I just took what we had before in this method

@mraspaud mraspaud requested a review from djhoese October 21, 2019 09:12
@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 86.094% when pulling 89b24b3 on mraspaud:feature-new-overlay into a6ce4df on pytroll:master.

@codecov
Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #934 into master will increase coverage by 0.79%.
The diff coverage is 83.09%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #934      +/-   ##
=========================================
+ Coverage   85.41%   86.2%   +0.79%     
=========================================
  Files         172     174       +2     
  Lines       26046   26791     +745     
=========================================
+ Hits        22246   23094     +848     
+ Misses       3800    3697     -103
Impacted Files Coverage Δ
satpy/tests/test_writers.py 97.92% <100%> (+0.11%) ⬆️
satpy/config.py 80.86% <71.42%> (-1.12%) ⬇️
satpy/writers/__init__.py 84.9% <71.42%> (-0.32%) ⬇️
satpy/tests/compositor_tests/test_viirs.py 98.71% <0%> (-0.59%) ⬇️
satpy/readers/nucaps.py 93.71% <0%> (-0.53%) ⬇️
satpy/tests/test_multiscene.py 97.84% <0%> (ø) ⬆️
satpy/multiscene.py 89.13% <0%> (ø) ⬆️
satpy/composites/ahi.py 100% <0%> (ø) ⬆️
satpy/readers/modis_l2.py 98.52% <0%> (ø) ⬆️
satpy/readers/seviri_l1b_hrit.py 92.3% <0%> (ø) ⬆️
... and 17 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 a6ce4df...89b24b3. Read the comment docs.

@@ -5,7 +5,7 @@ env:
- PYTHON_VERSION=$TRAVIS_PYTHON_VERSION
- NUMPY_VERSION=stable
- MAIN_CMD='python setup.py'
- CONDA_DEPENDENCIES='xarray!=0.13.0 dask distributed toolz Cython sphinx cartopy pillow matplotlib scipy pyyaml pyproj pyresample coveralls coverage codecov behave netcdf4 h5py h5netcdf gdal rasterio imageio pyhdf mock libtiff pycoast pydecorate geoviews zarr six'
- CONDA_DEPENDENCIES='xarray!=0.13.0 dask distributed toolz Cython sphinx cartopy pillow matplotlib scipy pyyaml pyproj pyresample coveralls coverage codecov behave netcdf4 h5py h5netcdf gdal rasterio imageio pyhdf mock libtiff geoviews zarr six'
Copy link
Member

Choose a reason for hiding this comment

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

Are these being implicitly installed with the test commands? Shouldn't these be added to the PIP_DEPENDENCIES?

Copy link
Member Author

@mraspaud mraspaud Oct 25, 2019

Choose a reason for hiding this comment

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

No, they are mocked now, so they aren't needed anymore for the tests

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting. Do we want them to be mocked?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for unit tests they should. But I also think we should have system tests (running every night) what would use actual pycoast and pydecorate to make sure compatibility isn't broken.
IMHO this has the following advantages:

  • we can continue having passing tests (and thus continue developping) even when an external lib changes its API
  • unit tests install and run faster
  • unit tests don't need to know the internals of the external lib, so it keeps the testing code cleaner
    BUT as said above, we then need to run system tests to make sure the whole system still works.

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.

My only remaining question besides the comment I just made a second ago is whether or not an issue should be made to move the logical defaults to pycoast? Otherwise LGTM.

@mraspaud
Copy link
Member Author

I'll make an issue, thanks for reviewing so fast !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:writers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants