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 sensor-name property to NWCSAF readers #1113

Merged
merged 10 commits into from Mar 18, 2020

Conversation

adybbroe
Copy link
Contributor

@adybbroe adybbroe commented Mar 12, 2020

Add the sensor name as a property to the NWCSAF/Geo&PPS readers

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe requested a review from mraspaud as a code owner March 12, 2020 15:02
@adybbroe adybbroe self-assigned this Mar 12, 2020
@coveralls
Copy link

coveralls commented Mar 12, 2020

Coverage Status

Coverage decreased (-0.008%) to 89.327% when pulling 8c8815c on adybbroe:fix-sensor-name-nwcsaf-readers into e91b6c7 on pytroll:master.

@codecov
Copy link

codecov bot commented Mar 12, 2020

Codecov Report

Merging #1113 into master will decrease coverage by <.01%.
The diff coverage is 94.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
- Coverage   89.33%   89.33%   -0.01%     
==========================================
  Files         195      195              
  Lines       28796    28827      +31     
==========================================
+ Hits        25726    25753      +27     
- Misses       3070     3074       +4
Impacted Files Coverage Δ
satpy/tests/reader_tests/test_nwcsaf_nc.py 100% <100%> (ø) ⬆️
satpy/readers/nwcsaf_nc.py 54.83% <87.5%> (+0.29%) ⬆️

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 e91b6c7...8c8815c. Read the comment docs.

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe
Copy link
Contributor Author

@djhoese and @mraspaud I restructured a bit in the reader, in order to more easily add the unittest. Don't know if it is to your liking?

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.

Thanks for making the code more modular! Just a few details to fix imo

satpy/readers/nwcsaf_nc.py Show resolved Hide resolved
satpy/readers/nwcsaf_nc.py Outdated Show resolved Hide resolved
satpy/readers/nwcsaf_nc.py Outdated Show resolved Hide resolved
satpy/tests/reader_tests/test_nwcsaf_nc.py Outdated Show resolved Hide resolved
@gerritholl
Copy link
Collaborator

It appears to still load seviri.yaml when I try this PR. The sensor seviri is (still) mentioned 121 times in satpy/etc/readers/nwcsaf-geo.yaml, but even if I remove those references it's still loading them, I'm not sure why.

@gerritholl
Copy link
Collaborator

gerritholl commented Mar 13, 2020

The ABI file is available from s3://noaa-goes16/ABI-L1b-RadF/2017/073/20/OR_ABI-L1b-RadF-M3C15_G16_s20170732006100_e20170732016473_c20170732016531.nc

The NWCSAF file:

S_NWC_CMIC_GOES16_NEW-ENGLAND-NR_20170314T200610Z.nc.gz

@adybbroe
Copy link
Contributor Author

@gerritholl I will have a look at it thanks!

@gerritholl
Copy link
Collaborator

This should probably be turned into a unit test or so, it should pass and currently fails:

import satpy
from satpy.utils import debug_on
debug_on()
sc = satpy.Scene(
    {"abi_l1b": ['import/Sat_data/OR_ABI-L1b-RadF-M3C02_G16_s20170732006100_e20170732016467_c20170732016504.nc'],
     "nwcsaf-geo": ['export/CMIC/S_NWC_CMIC_GOES16_NEW-ENGLAND-NR_20170314T200610Z.nc']})
assert "seviri" not in sc.attrs["sensor"]

@adybbroe
Copy link
Contributor Author

@gerritholl I tested the code snippet above. Works for me!?

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@ghost
Copy link

ghost commented Mar 13, 2020

Congratulations 🍻. DeepCode analyzed your code in 0.226 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe requested a review from pnuu as a code owner March 13, 2020 15:37
@adybbroe
Copy link
Contributor Author

adybbroe commented Mar 13, 2020

Ok, so now your ABI code above should work @gerritholl
I tested this locally:

import satpy
from satpy.utils import debug_on
debug_on()
sc = satpy.Scene(
    {"abi_l1b": ['/home/a000680/data/goes/abi/OR_ABI-L1b-RadF-M3C15_G16_s20170732006100_e20170732016473_c20170732016531.nc'],
     "nwcsaf-geo": ['/home/a000680/data/goes/nwcsaf/S_NWC_CMIC_GOES16_NEW-ENGLAND-NR_20170314T200610Z.nc']})
print(sc.attrs['sensor'])
sc.load(['cloud_top_phase', 'C15'])
areaid = 'new-england-2000'
lc = sc.resample(areaid, radius_of_influence=20000)
lc.show('cloud_top_phase')
lc.show('C15')

And it works. Using your area-def I got on slack.

I suppose we should move the NWCSAF product recipes into visir.py in another PR.
Any opinion on this @djhoese, @mraspaud, @pnuu and @sfinkens ?

Adam.Dybbroe added 2 commits March 13, 2020 16:52
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Implement recommendations from reviewer

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Copy link
Collaborator

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

satpy/etc/composites/seviri.yaml Outdated Show resolved Hide resolved
satpy/etc/composites/visir.yaml Outdated Show resolved Hide resolved
satpy/etc/readers/nwcsaf-geo.yaml Outdated Show resolved Hide resolved
satpy/readers/nwcsaf_nc.py Outdated Show resolved Hide resolved
satpy/etc/readers/nwcsaf-geo.yaml Show resolved Hide resolved
Adam.Dybbroe added 2 commits March 16, 2020 16:22
…eaving such a move to another PR

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe requested a review from mraspaud March 17, 2020 13:14
@mraspaud mraspaud changed the title Add sensor-name property tp NWCSAF readers Add sensor-name property to NWCSAF readers Mar 17, 2020
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, but I still see a blank line ?

satpy/etc/readers/nwcsaf-geo.yaml Outdated Show resolved Hide resolved
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe added the bug label Mar 17, 2020
@adybbroe adybbroe added component:compositors enhancement code enhancements, features, improvements labels Mar 17, 2020
@mraspaud mraspaud merged commit ec24bcc into pytroll:master Mar 18, 2020
@adybbroe adybbroe deleted the fix-sensor-name-nwcsaf-readers branch March 18, 2020 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component:compositors enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loading both abi and nwcsaf-geo confuses satpy into sometimes trying the wrong composite
4 participants