Add meirink calib #2589

pdebuyl · 2023-10-05T14:42:09Z

Closes Add Calibration by Meirink et al for SEVIRI #2571
Tests added
Fully documented

pdebuyl · 2023-10-05T15:13:16Z

Hello, this is more a "proof of concept" in terms of structure. I "hacked" the gain in internal_gain to avoid having to decide for a completely new logic. -> feedback welcome on this. If we want more generic custom calibrations, this will require thinking in any case.

(docs missing)

sfinkens

Awesome, thanks for starting this! 👍 Looks good aready! I think since the coefficients are not within the file and also static, everything could be delegated to a "meirink calibration handler". For example:

class MeirinkCalibrationHandler:
    def __init__(self, coefs=MEIRINK_COEFS):
        self.coefs = coefs

    def get_slope(self, platform, channel, time, version):
        coefs = self.coefs[version][platform][channel]
        return get_meirink_slope(coefs, time)

    def get_version(self, calib_mode):
        return calib_mode.split("-")[1]

class SEVIRICalibrationHandler:
    def get_gain_offset(...):
        ...
        if "MEIRINK" in calib_mode:
            meirink = MeirinkCalibrationHandler()
            version = meirink.get_version(calib_mode)
            internal_gain = meirink.get_slope(version, self._platform_id, self._channel_name, self._scan_time)

I'm assuming here that the calibration mode includes the version calib_mode=meirink-2023 (as you proposed) and the coefficient dictionary has one extra layer for the version.

Maybe that's already enough for one PR. Channel specific coefficients are worth a separate PR and probably need more discussion on slack.

satpy/readers/seviri_base.py

satpy/tests/reader_tests/test_seviri_base.py

(typos from copying the coefficients over)

pdebuyl · 2023-10-06T12:18:52Z

I put fixed values for the test, can you check it?

https://github.com/pdebuyl/satpy/blob/add_meirink_calib/satpy/tests/reader_tests/test_seviri_base.py#L379

I only check for the slope and not the offset, as the offset is taken from the input file and is not available within the test environment.

pdebuyl · 2023-10-06T12:23:09Z

To generate the tests, I wrote a sort parser for the text file that is now on the website of the MSG CPP team.

https://msgcpp.knmi.nl/assets/custom_cal_aqua_c6_abs.txt

The code:

from datetime import datetime

with open('custom_cal_aqua_c6_abs.txt', 'r') as f:
    lines = filter(lambda x: not x.strip().startswith('#'), f.readlines())


coef = {'Met8': {},'Met9': {},'Met10': {},'Met11': {}}
channels = ['VIS006', 'VIS008', 'IR_016']
for k in coef.keys():
    for c in channels:
        coef[k][c] = []
for oneline in lines:
    oneline = oneline.split()
    platform = oneline.pop(0).strip()
    for c in channels:
        coef[platform][c].append(float(oneline.pop(0)))


def get_slope(platform, channel, time):
    A, B = coef[platform][channel]
    DATE_2000 = datetime(2000, 1, 1)
    delta_t = (time - DATE_2000).total_seconds()
    S = A + B * 1.e-3*delta_t / 24 / 3600
    return S/1000


examples = [
    ('Met8', datetime(2005, 1, 18)),
    ('Met8', datetime(2010, 12, 31)),
    ('Met9', datetime(2010, 1, 18)),
    ('Met9', datetime(2015, 6, 1)),
    ('Met10', datetime(2005, 1, 18)),
    ('Met10', datetime(2010, 12, 31)),
    ('Met11', datetime(2010, 1, 18)),
    ('Met11', datetime(2015, 6, 1)),
]

for platform, time in examples:
    print(f"({platform},  {time.__repr__()}, ", [get_slope(platform, channel, time) for channel in channels], "),")

sfinkens · 2023-10-09T07:28:56Z

Coefficients look good now, thanks!

pdebuyl · 2023-10-09T12:31:48Z

I added a comment for the units.

Regarding the rest of your comment, it would be best to take them into account separately or after a discussion on slack, it this correct?

Returning a dict would only have an effect on the content of the calibrate routine if I understand well. If the scope of the data passing is only with the seviri handler itself and not part of an API is it interesting to do?

I have a draft with your proposed MeirinkCalibrationHandler which I can share if you want.

The dict is defined along MEIRINK_COEFS['2013'][322] = {'VIS006': (21.026, 0.2556), (...)}

sfinkens · 2023-10-09T12:52:17Z

I added a comment for the units.

Thanks!

Regarding the rest of your comment, it would be best to take them into account separately or after a discussion on slack, it this correct?

Yes I think so. There are probably some use cases or implications that I'm not aware of.

Returning a dict would only have an effect on the content of the calibrate routine if I understand well. If the scope of the data passing is only with the seviri handler itself and not part of an API is it interesting to do?

That's correct, I just thought it would improve readability 🙂

I have a draft with your proposed MeirinkCalibrationHandler which I can share if you want.

Yes please! I think that would fit well in this PR.

Co-authored-by: Stephan Finkensieper <stephan.finkensieper@dwd.de>

satpy/readers/seviri_base.py

sfinkens · 2023-10-09T14:40:45Z

Looks very nice, just nitpicking now

Also fix typo

pdebuyl · 2023-10-10T09:38:07Z

Hi @sfinkens I only left out the dict-type for the reply of get_gain_offset so far.

As mentioned above, I have no preference for the API of MeirinkCalibrationHandler let me know if this needs to be discussed.

mraspaud

Alright, looks good! Just a couple of small comments.

satpy/readers/seviri_base.py

mraspaud · 2023-10-10T13:19:08Z

Oh, tests are failing...

mraspaud · 2023-10-10T20:06:20Z

@pdebuyl thanks a lot for adding this! Only thing left for me are the failing tests, otherwise this looks good to me

codecov · 2023-10-11T08:17:04Z

Codecov Report

Merging #2589 (056a3a9) into main (666dcaa) will increase coverage by 0.03%.
Report is 108 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2589      +/-   ##
==========================================
+ Coverage   94.91%   94.94%   +0.03%     
==========================================
  Files         351      354       +3     
  Lines       51215    51474     +259     
==========================================
+ Hits        48611    48873     +262     
+ Misses       2604     2601       -3

Flag	Coverage Δ
behaviourtests	`4.27% <0.00%> (-0.03%)`	⬇️
unittests	`95.56% <100.00%> (+0.04%)`	⬆️

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

Files	Coverage Δ
satpy/readers/seviri_base.py	`100.00% <100.00%> (ø)`
satpy/tests/reader_tests/test_seviri_base.py	`100.00% <100.00%> (ø)`

... and 23 files with indirect coverage changes

pdebuyl · 2023-10-11T08:41:13Z

Hello @mraspaud thanks for also checking this.

Version 3.11 still fails in the CI, no idea why.

I also added a short documentation in the file.

mraspaud · 2023-10-11T09:41:21Z

Really weird, windows is behaving...

mraspaud · 2023-10-11T09:43:50Z

I triggered a rerun of the failed jobs, as I don't see how the changes here could affect the failing tests, and the tests pass correctly in other PRs. So I hope for a random glitch

pdebuyl · 2023-10-11T10:08:27Z

there is one "isort" issue in satpy/tests/reader_tests/test_seviri_base.py. I wait for all tests to complete though.

The failure in "CI / test (3.11, ubuntu-latest, true) (pull_request)" is a NumPy 2.0 issue.

pdebuyl · 2023-10-11T10:10:49Z

pushing the isort fix

mraspaud · 2023-10-11T10:47:59Z

The failure in "CI / test (3.11, ubuntu-latest, true) (pull_request)" is a NumPy 2.0 issue.

Yes, that's the unstable build, and we are aware of the problems there, but it's not blocking (ie we'll merge even if this is failing)

pdebuyl · 2023-10-11T10:48:32Z

OK, I have no further idea for the two failing tests. One is NumPy 2.0, which I guess is out of scope. The other is about the lack of type annotation for MEIRINK_COEFS, which I don't think is required by satpy :-s

mraspaud · 2023-10-11T11:00:22Z

Yes, the type annotation for pre-commit is strange. I'm not aware that we are now requiring type annotation... @djhoese do you have an idea what this is about?

The coefficients on the webpage https://msgcpp.knmi.nl/solar-channel-calibration.html were updated in place. The current set of coefficients were obtained in 2023, the code now reflects this correctly.

pdebuyl · 2023-10-11T12:21:15Z

BTW I changed the "version" of MEIRINK-2013 to MEIRINK-2023 because the KNMI actually puts the updates of the coefficients "in place" on their web page. (see discussion on slack, but this is all in the PR message and added in the file for information).

sfinkens

LGTM, thanks a lot!

djhoese · 2023-10-11T15:48:15Z

Made a commit to make pre-commit's mypy (type annotations) checks work.

mraspaud · 2023-10-11T15:51:38Z

Thanks @djhoese, but that looks like mypy is misbehaving. We have not configured it to enforce annotations, right?

djhoese · 2023-10-11T15:53:17Z

Yes we are. It is in pre-commit. The rest of satpy was made compliant (by me). And yes we've had this discussion before. Mypy will usually skip over functions/methods that don't have type annotations in the declaration, but I don't think that applies to globals. I think the complex structure of the content of this particular dictionary was just too much for mypy to guess at.

mraspaud · 2023-10-11T15:54:20Z

ok...

pdebuyl · 2023-10-11T17:20:15Z

Only "3.11 ubuntu-latest true" failing.

KeyError for orbital parameters at https://github.com/pytroll/satpy/actions/runs/6484922970/job/17609898950?pr=2589#step:10:268

Is this related to my PR?

mraspaud · 2023-10-11T17:37:04Z

@pdebuyl it's not your PR, I'm merging this.

pdebuyl · 2023-10-11T19:28:30Z

Cool :-) A good day to contribute to satpy!

pdebuyl added 2 commits October 5, 2023 13:10

add meirink calibration method

785f1c5

add test for the slope of the Meirink coefficients

984a733

pdebuyl requested review from sfinkens, mraspaud, adybbroe and djhoese as code owners October 5, 2023 14:42

remove un-necessary import

a9521ef

sfinkens reviewed Oct 6, 2023

View reviewed changes

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

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

satpy/tests/reader_tests/test_seviri_base.py Outdated Show resolved Hide resolved

pdebuyl added 2 commits October 6, 2023 11:18

fix MEIRINK calibration coefficients

688eb79

(typos from copying the coefficients over)

used fixed values to test meirink calibration

0abbb9a

pdebuyl added 2 commits October 6, 2023 14:24

isort

909fb05

fix "multiple spaces after ','" for codefactor

5c22342

add comment with the units of the Meirink coefficients

09efba8

Use MeirinkCalibrationHandler to manage the coefficients

8193298

Co-authored-by: Stephan Finkensieper <stephan.finkensieper@dwd.de>

sfinkens reviewed Oct 9, 2023

View reviewed changes

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

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

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

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

pdebuyl added 2 commits October 9, 2023 23:22

Move criterion for application of meirink calibration in method

ad1b690

Also fix typo

flake8

65503a0

mraspaud reviewed Oct 10, 2023

View reviewed changes

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

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

pdebuyl added 2 commits October 10, 2023 16:00

rename DATE_2000 to MEIRINK_EPOCH

dbefc0f

remove coef argument in MeirinkCalibrationHandler

60bb30f

mraspaud added the enhancement code enhancements, features, improvements label Oct 10, 2023

mraspaud added the component:readers label Oct 10, 2023

mraspaud assigned pdebuyl Oct 10, 2023

add documentation about Meirink calibration

69c5751

mraspaud added this to the v0.44.0 milestone Oct 11, 2023

Update test for changes in Meirink calibration

714130c

isort

51a3331

Change version of Meirink coefficients to 2023.

3b465a0

The coefficients on the webpage https://msgcpp.knmi.nl/solar-channel-calibration.html were updated in place. The current set of coefficients were obtained in 2023, the code now reflects this correctly.

sfinkens approved these changes Oct 11, 2023

View reviewed changes

Fix type annotations in seviri_base.py

056a3a9

mraspaud merged commit aaddc82 into pytroll:main Oct 11, 2023
19 of 20 checks passed

pdebuyl deleted the add_meirink_calib branch October 11, 2023 19:28

sfinkens mentioned this pull request Oct 12, 2023

Allow channel-specific calibration mode for SEVIRI #2599

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add meirink calib #2589

Add meirink calib #2589

pdebuyl commented Oct 5, 2023 •

edited by mraspaud

Loading

pdebuyl commented Oct 5, 2023

sfinkens left a comment

pdebuyl commented Oct 6, 2023

pdebuyl commented Oct 6, 2023

sfinkens commented Oct 9, 2023

pdebuyl commented Oct 9, 2023

sfinkens commented Oct 9, 2023 •

edited

Loading

sfinkens commented Oct 9, 2023

pdebuyl commented Oct 10, 2023

mraspaud left a comment

mraspaud commented Oct 10, 2023

mraspaud commented Oct 10, 2023

codecov bot commented Oct 11, 2023 •

edited

Loading

pdebuyl commented Oct 11, 2023

mraspaud commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

sfinkens left a comment

djhoese commented Oct 11, 2023

mraspaud commented Oct 11, 2023

djhoese commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

Add meirink calib #2589

Add meirink calib #2589

Conversation

pdebuyl commented Oct 5, 2023 • edited by mraspaud Loading

pdebuyl commented Oct 5, 2023

sfinkens left a comment

Choose a reason for hiding this comment

pdebuyl commented Oct 6, 2023

pdebuyl commented Oct 6, 2023

sfinkens commented Oct 9, 2023

pdebuyl commented Oct 9, 2023

sfinkens commented Oct 9, 2023 • edited Loading

sfinkens commented Oct 9, 2023

pdebuyl commented Oct 10, 2023

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud commented Oct 10, 2023

mraspaud commented Oct 10, 2023

codecov bot commented Oct 11, 2023 • edited Loading

Codecov Report

pdebuyl commented Oct 11, 2023

mraspaud commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

sfinkens left a comment

Choose a reason for hiding this comment

djhoese commented Oct 11, 2023

mraspaud commented Oct 11, 2023

djhoese commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

mraspaud commented Oct 11, 2023

pdebuyl commented Oct 11, 2023

pdebuyl commented Oct 5, 2023 •

edited by mraspaud

Loading

sfinkens commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading