Harmonize calibration in SEVIRI readers #1457

sfinkens · 2020-11-27T09:26:08Z

Harmonize calibration in the SEVIRI HRIT, Native and netCDF format readers:

All readers now accept an ext_calib_coefs kwarg for external calibration coefficients. Switching between nominal and GSICS calibration mode is possible for HRIT and Native. netCDF files only provides one set of coefficients.
Move calibration to independent classes
1. SEVIRICalibrationAlgorithm which provides the fundamental calibration algorithms
2. SEVIRICalibrationHandler which handles selection of calibration coefficients and calls the appropriate
  calibration algorithm.
Move calibration from get_dataset to a dedicated calibrate method, which uses _get_calib_coefs to read coefficients from the file and passes them to the calibration handler. The file handlers do not inherit from the calibration class anymore. That resolves the problem of the calibration class expecting the file handler to set certain attributes (for example Standardise self.mda for SEVIRI attributes #722).

In order to ensure code integrity, comprehensive calibration tests have been added before refactoring.

They passed before refactoring (84ff1f2; checks)
And they pass after refactoring (6f51d30)

TODO:

Fix/Remove old calibration tests
Add tests for coefficient selection
Update documentation

Tests added
Passes flake8 satpy
Fully documented

Use nominal calibration method instead of GSICS. Otherwise offset will be multiplied by gain.

ghost · 2020-12-01T10:38:07Z

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

👉 View analysis in DeepCode’s Dashboard | Configure the bot

codecov · 2020-12-01T12:40:20Z

Codecov Report

Merging #1457 (1f7d05b) into master (64fc4ef) will increase coverage by 0.08%.
The diff coverage is 96.41%.

@@            Coverage Diff             @@
##           master    #1457      +/-   ##
==========================================
+ Coverage   90.78%   90.87%   +0.08%     
==========================================
  Files         241      241              
  Lines       35111    35113       +2     
==========================================
+ Hits        31876    31908      +32     
+ Misses       3235     3205      -30

Flag	Coverage Δ
behaviourtests	`4.49% <0.00%> (-0.01%)`	⬇️
unittests	`91.34% <96.41%> (+0.08%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/tests/reader_tests/test_seviri_base.py	`100.00% <ø> (ø)`
satpy/readers/seviri_l1b_hrit.py	`90.87% <72.72%> (-1.03%)`	⬇️
satpy/readers/seviri_base.py	`95.26% <100.00%> (+1.61%)`	⬆️
satpy/readers/seviri_l1b_native.py	`79.31% <100.00%> (+0.93%)`	⬆️
satpy/readers/seviri_l1b_nc.py	`50.00% <100.00%> (+6.89%)`	⬆️
.../tests/reader_tests/test_seviri_l1b_calibration.py	`100.00% <100.00%> (+3.27%)`	⬆️
satpy/tests/reader_tests/test_seviri_l1b_hrit.py	`100.00% <100.00%> (ø)`
satpy/tests/reader_tests/test_seviri_l1b_native.py	`99.41% <100.00%> (+4.33%)`	⬆️
satpy/tests/reader_tests/test_seviri_l1b_nc.py	`100.00% <100.00%> (ø)`
... and 2 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 64fc4ef...1f7d05b. Read the comment docs.

satpy/readers/seviri_base.py

satpy/readers/seviri_l1b_nc.py

mraspaud

Very good work on refactoring all the calibration and tests into a common ground for the various seviri reader! I have a few style comment, but otherwise LGTM!

satpy/readers/seviri_base.py

satpy/readers/seviri_l1b_hrit.py

satpy/readers/seviri_l1b_native.py

satpy/readers/seviri_base.py

mraspaud

LGTM, thanks for the comprehensive overhaul of the tests!

ameraner

Looks very good to me! Thanks for sorting this out.

PS: first time I see np.choose being used, nice!

sjoro

looks good to me! very nice restructuring, the logic is a lot easier to follow now. thanks a lot for putting in the effort on sorting this out!

simonrp84

Nice work, @sfinkens! I have two relatively minor comments, otherwise all looks good.

satpy/readers/seviri_base.py

satpy/readers/seviri_l1b_hrit.py

Furthermore, add HRV calibration tests

Check if they are zero instead of checking the channel name.

sfinkens · 2020-12-15T10:05:36Z

Looks like the undefined loop variable warning from CodeFactor was already there in #1438 (https://github.com/pytroll/satpy/pull/1438/checks?check_run_id=1525081771). @strandgren Was that intended?

strandgren · 2020-12-15T10:12:24Z

Looks like the undefined loop variable warning from CodeFactor was already there in #1438 (https://github.com/pytroll/satpy/pull/1438/checks?check_run_id=1525081771). @strandgren Was that intended?

Yes, because there is a check in the ImageBoundaries class to make sure that the image boundaries are valid. So there should be no risk that these variables are undefined. We agreed on that this solves the problem, but that the bots cannot follow the solution.

This is also a good place for the common documentation.

This reverts commit dfb9d73.

- Move common documentation to seviri_base module Main documentation: - Create separate section for SEVIRI L1.5 readers - Include seviri_base as well as all L1.5 readers

mraspaud

Looks great, thanks for adding the docstrings to the main documentation.

Add comprehensive calib. test for HRIT reader

3569be4

sfinkens mentioned this pull request Nov 27, 2020

Add comprehensive calibration tests for SEVIRI readers #1458

Closed

5 tasks

sfinkens changed the title ~~Harmonize SEVIRI HRIT and Native format readers~~ Harmonize SEVIRI HRIT/Native/NC readers Nov 30, 2020

sfinkens added 7 commits November 30, 2020 19:08

Generalize calibration test

3fe18a3

Add comprehensive calib. test for Native reader

f3e30e1

Add comprehensive calib. test for netCDF reader

84ff1f2

Harmonize calib. of SEVIRI HRIT and Native readers

ab0f489

Separate calibration from file handler

0e5a01c

Update SEVIRI nc reader as well

18a6c16

Fix netCDF reader

2e45029

Use nominal calibration method instead of GSICS. Otherwise offset will be multiplied by gain.

sfinkens force-pushed the feature-align-seviri-readers branch from ea9ca06 to 2e45029 Compare November 30, 2020 19:38

sfinkens changed the title ~~Harmonize SEVIRI HRIT/Native/NC readers~~ Harmonize calibration in SEVIRI HRIT/Native/NC readers Nov 30, 2020

sfinkens changed the title ~~Harmonize calibration in SEVIRI HRIT/Native/NC readers~~ Harmonize calibration in SEVIRI readers Nov 30, 2020

sfinkens added 3 commits November 30, 2020 19:53

Enable all tests for Native and NC readers

c188edf

Remove old calibration tests

eb6691a

Add more calibration tests

89b48ce

sfinkens added 2 commits December 1, 2020 12:31

Repair calibration algorithm tests

afb3d90

Move calibration algorithm to separate class

6f51d30

sfinkens mentioned this pull request Dec 1, 2020

Standardise self.mda for SEVIRI attributes #722

Closed

sfinkens marked this pull request as ready for review December 1, 2020 14:00

sfinkens requested review from adybbroe, djhoese and mraspaud as code owners December 1, 2020 14:00

sfinkens self-assigned this Dec 1, 2020

sfinkens added component:readers enhancement code enhancements, features, improvements labels Dec 1, 2020

sfinkens commented Dec 1, 2020

View reviewed changes

satpy/readers/seviri_base.py Outdated Show resolved Hide resolved

sfinkens commented Dec 1, 2020

View reviewed changes

satpy/readers/seviri_l1b_nc.py Outdated Show resolved Hide resolved

mraspaud reviewed Dec 1, 2020

View reviewed changes

satpy/readers/seviri_base.py Outdated Show resolved Hide resolved

satpy/readers/seviri_l1b_hrit.py Outdated Show resolved Hide resolved

satpy/readers/seviri_l1b_native.py Outdated Show resolved Hide resolved

satpy/readers/seviri_base.py Outdated Show resolved Hide resolved

sfinkens added 6 commits December 2, 2020 16:13

Balance level of abstraction in calibrate

d63017d

Test data types in calibration

6b00bf0

Remove duplicate thresholding

90f3eb9

Privatize attributes of calibration classes

ce18171

Factorize creation of coefficient dictionary

e9fd289

Document meaning of line quality flags

996cab2

This was referenced Dec 8, 2020

Add support for SEVIRI Native foua-pps/level1c4pps#50

Merged

SEVIRI Native data for CLAAS-3 cmsaf/satpy#1

Closed

mraspaud approved these changes Dec 8, 2020

View reviewed changes

ameraner approved these changes Dec 8, 2020

View reviewed changes

sjoro approved these changes Dec 9, 2020

View reviewed changes

simonrp84 suggested changes Dec 10, 2020

View reviewed changes

satpy/readers/seviri_base.py Outdated Show resolved Hide resolved

satpy/readers/seviri_l1b_hrit.py Show resolved Hide resolved

sfinkens added 3 commits December 15, 2020 09:12

Merge branch 'master' into feature-align-seviri-readers

00a8863

Fix HRV coefficient selection in HRIT

31419c0

Furthermore, add HRV calibration tests

Improve check for GSICS availability

916b297

Check if they are zero instead of checking the channel name.

sfinkens added 5 commits December 15, 2020 11:46

Add common base class for initialization

dfb9d73

This is also a good place for the common documentation.

Revert "Add common base class for initialization"

088956f

This reverts commit dfb9d73.

Update documentation

9760c44

- Move common documentation to seviri_base module Main documentation: - Create separate section for SEVIRI L1.5 readers - Include seviri_base as well as all L1.5 readers

Update URLs of reference documents

c2bd336

Explain why there's no calib_mode in seviri_nc

1f7d05b

mraspaud approved these changes Dec 15, 2020

View reviewed changes

mraspaud merged commit 45843de into pytroll:master Dec 16, 2020

sfinkens deleted the feature-align-seviri-readers branch December 16, 2020 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harmonize calibration in SEVIRI readers #1457

Harmonize calibration in SEVIRI readers #1457

sfinkens commented Nov 27, 2020 •

edited

Loading

ghost commented Dec 1, 2020 •

edited by ghost

Loading

codecov bot commented Dec 1, 2020 •

edited

Loading

mraspaud left a comment

mraspaud left a comment

ameraner left a comment

sjoro left a comment

simonrp84 left a comment

sfinkens commented Dec 15, 2020

strandgren commented Dec 15, 2020

mraspaud left a comment

Harmonize calibration in SEVIRI readers #1457

Harmonize calibration in SEVIRI readers #1457

Conversation

sfinkens commented Nov 27, 2020 • edited Loading

ghost commented Dec 1, 2020 • edited by ghost Loading

👉 View analysis in DeepCode’s Dashboard | Configure the bot

codecov bot commented Dec 1, 2020 • edited Loading

Codecov Report

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

ameraner left a comment

Choose a reason for hiding this comment

sjoro left a comment

Choose a reason for hiding this comment

simonrp84 left a comment

Choose a reason for hiding this comment

sfinkens commented Dec 15, 2020

strandgren commented Dec 15, 2020

mraspaud left a comment

Choose a reason for hiding this comment

sfinkens commented Nov 27, 2020 •

edited

Loading

ghost commented Dec 1, 2020 •

edited by ghost

Loading

codecov bot commented Dec 1, 2020 •

edited

Loading