Fix osisaf SST reader #1410

adybbroe · 2020-10-20T19:20:04Z

Adding support for reading OSISAF GHRSST formattet SST data.
There was some code to read the OSISAF SST but wasn't really finished. Further the SLSTR-L2 reader was actually a GHRSST reader, and both OSISAF and SLSTR-L2 follow the GHRSST format standard. So we can make use of the same code.

The satpy/etc/readers/ghrsst_l3c_sst.yamlhas been removed and replaced by ghrsst_l2.yaml
The reader slstr_l2.py has been replaced by ghrsst_l2.py, which is only a slightly modified/enhanced compared to slstr_l2.py
Renamed the test module test_slstr_l2.py to test_ghrsst_l2.py
And finally I have added an enhancement in generic.yaml to display the SST using a colormap - I suppose we should remove that and instead suggest how to do this via the documentation or the pytroll-examples?

Closes #xxxx
Tests added
Passes flake8 satpy
Fully documented

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

adybbroe · 2020-10-20T19:25:37Z

I have two yaml files, one for OSISAF (ghrsst_l2.yaml) and one for SLSTR (the old name slstr_l2). Both use the same python code. But the SLSTR one has sensors = slstr obviously. The OSISAF SST is one product but from either VIIRS or AVHRR.
Should we try have one yaml file anyhow, and how do we deal with the sensors attribute then? Or should we keep two yaml files? If yes to the latter I guess we/I should rename them?

mraspaud · 2020-10-21T07:10:51Z

iirc you can add a sensor property to the file handler, so you would need only one yaml file.

adybbroe · 2020-10-21T09:45:55Z

Ok, thanks @mraspaud
But wouldn't that mean I would have two entries for the same sea_surface_temperature filed?
Or do you mean something like this:

  sea_surface_temperature_osisaf:
    name: sea_surface_temperature
    coordinates: [longitude_osisaf, latitude_osisaf]
    file_type: GHRSST_OSISAF
    resolution: 2000
    units: kelvin
    standard_name: sea_surface_temperature

  sea_surface_temperature_slstr:
    name: sea_surface_temperature
    sensor: slstr
    coordinates: [longitude_slstr, latitude_slstr]
    file_type: SLSTR
    resolution: 1000
    view: nadir
    units: kelvin
    standard_name: sea_surface_temperature

?

mraspaud · 2020-10-21T14:55:48Z

no I mean in the filehandler python code

…th SLSTR and OSISAF Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

ghost · 2020-10-21T18:36:23Z

DeepCode's analysis on #1d2e85 found:

ℹ️ 1 minor issue. 👇

Top issues

Description	Example fixes
standard import "import tarfile" should be placed before "from satpy.readers.file_handlers import BaseFileHandler" Occurrences: ghrsst_l2.py:23 ghrsst_l2.py:24 ghrsst_l2.py:25	🔧 Example fixes

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

adybbroe · 2020-10-21T18:37:55Z

I see what you mean @mraspaud
Not sure though how that is then used with advantage in the yaml file.
Have created one yaml file for both products (OSISAF & SLSTR) ad I can read and project and display both.
But the yaml file seems a bit clumsy still

# Conflicts: # satpy/etc/readers/slstr_l2.yaml # satpy/readers/ghrsst_l2.py Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

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

Color legend originally provided by MET Norway and the OSISAF

adybbroe · 2022-03-25T13:39:42Z

This code now produce the image below:

    scn = Scene(filenames=OSISAF_SST_FILES, reader='ghrsst_l2')
    scn.load(['sea_surface_temperature'])
    local_scn = scn.resample('euron1')
    local_scn.show('sea_surface_temperature')

Using the enhancement here:

  osisaf_sst_smhi_colormap:
    standard_name: sea_surface_subskin_temperature
    operations:
    - name: osisaf_sst
      method: !!python/name:satpy.enhancements.colorize
      kwargs:
          palettes:
            - colors: [
              [255, 0, 255],
              [195, 0, 129],
              [129, 0, 47],
              [195, 0, 0],
              [255, 0, 0],
              [236, 43, 0],
              [217, 86, 0],
              [200, 128, 0],
              [211, 154, 13],
              [222, 180, 26],
              [233, 206, 39],
              [244, 232, 52],
              [255.99609375, 255.99609375, 63.22265625],
              [203.125, 255.99609375, 52.734375],
              [136.71875, 255.99609375, 27.34375],
              [0, 255.99609375, 0],
              [0, 207.47265625, 0],
              [0, 158.94921875, 0],
              [0, 110.42578125, 0],
              [0, 82.8203125, 63.99609375],
              [0, 55.21484375, 127.9921875],
              [0, 27.609375, 191.98828125],
              [0, 0, 255.99609375],
              [100.390625, 100.390625, 255.99609375],
              [150.5859375, 150.5859375, 255.99609375]]
              min_value: 296.55
              max_value: 273.55

It is supposed to look as similar as possible to the imagery we currently produce at SMHI with mpop. The corresponding image and its diff is added below:

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

adybbroe · 2022-03-31T14:16:23Z

I have added a PR (#2075) documenting how the use of a colorize enhancement as above can be done. So I am ready to remove the part in the enhancement.yaml file. I have, however, also asked the OSISAF if they could confirm the coloring above or provide what they regard as the standard. In which case I think it would be fair to include it among the default enhancements. Agree?

adybbroe · 2022-04-02T11:32:44Z

Got an answer from our colleagues in the OSISAF. There is yet no standard/official OSISAF color scale to refer to, so I suggest we remove it from the general.yaml file in this PR.

mraspaud

Thanks for this PR, looks promising. Some inline comments.

satpy/etc/enhancements/generic.yaml

satpy/readers/ghrsst_l2.py

satpy/etc/readers/ghrsst_l2.yaml

Follow Python 3 standard for class inherritance Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>

… reading Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

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

codecov · 2022-04-06T07:46:24Z

Codecov Report

Merging #1410 (e3d7dda) into main (d4c4332) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1410      +/-   ##
==========================================
+ Coverage   93.86%   93.89%   +0.02%     
==========================================
  Files         283      283              
  Lines       42379    42583     +204     
==========================================
+ Hits        39781    39985     +204     
  Misses       2598     2598

Flag	Coverage Δ
behaviourtests	`4.70% <0.00%> (-0.03%)`	⬇️
unittests	`94.45% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/composites/__init__.py	`89.94% <ø> (ø)`
satpy/readers/ghrsst_l2.py	`100.00% <100.00%> (ø)`
satpy/tests/reader_tests/test_ghrsst_l2.py	`100.00% <100.00%> (ø)`
satpy/readers/ghrsst_l3c_sst.py	`0.00% <0.00%> (-7.05%)`	⬇️
satpy/modifiers/_crefl.py	`92.40% <0.00%> (-0.62%)`	⬇️
satpy/tests/test_crefl_utils.py	`100.00% <0.00%> (ø)`
satpy/readers/seviri_l1b_hrit.py	`90.14% <0.00%> (ø)`
satpy/tests/reader_tests/test_utils.py	`100.00% <0.00%> (ø)`
satpy/tests/reader_tests/test_fci_l1c_nc.py	`100.00% <0.00%> (ø)`
satpy/tests/reader_tests/test_viirs_l1b.py	`97.26% <0.00%> (+0.06%)`	⬆️
... and 7 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 d4c4332...e3d7dda. Read the comment docs.

coveralls · 2022-04-06T08:09:16Z

Coverage increased (+0.03%) to 94.397% when pulling e3d7dda on adybbroe:fix-osisaf-reader into d4c4332 on pytroll:main.

… object instead Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

adybbroe · 2022-04-07T17:32:22Z

I tested the reader on the SLSTR files of this type: S3A_SL_2_WST____20220406T095733_20220406T100033_20220406T120437_0179_084_022_2340_ MAR_O_NR_003.SEN3.tar

and on the OSISAF netCDF files of this type: S-OSI_-FRA_-MTOP-NARSST_FIELD-202204061000Z.nc

Here are the results of both at the approximate same time morning April 6th, 2022 over Scandinavia (SLSTR to the right):

sfinkens

Nice work! Just a couple questions about readability.

sfinkens · 2022-04-08T12:57:41Z

satpy/readers/ghrsst_l2.py

            with tempfile.TemporaryDirectory() as tempdir:
                with tarfile.open(name=filename, mode='r') as tf:
                    sst_filename = next((name for name in tf.getnames()
-                                        if name.endswith('nc') and 'GHRSST-SSTskin' in name))
+                                         if name.endswith('nc') and 'GHRSST-SSTskin' in name))


You could move the if condition to a separate method, _is_sst_file for instance

Good suggestion, I will fix

sfinkens · 2022-04-08T12:59:04Z

satpy/tests/reader_tests/test_ghrsst_l2.py

+        mocked_dataset.assert_called()
+        mocked_dataset.reset_mock()
+
+        with patch('tarfile.open') as tf:


Instead of mocking, would it be possible to write such a tarfile during test setup?

Yes, I think this is a good idea. I will come back with a proposal

satpy/tests/reader_tests/test_ghrsst_l2.py

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

sfinkens

LGTM, thanks for refactoring the tests! Just two tiny comments

satpy/tests/reader_tests/test_ghrsst_l2.py

satpy/readers/ghrsst_l2.py

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

adybbroe · 2022-04-11T15:11:38Z

@mraspaud Are my updates satisfactory do you think?

mraspaud · 2022-04-12T07:12:41Z

yes, it look good now, but the tests are failing on windows, because of the classical permission error. However, as we are moving to pytest more now, I would suggest to transition the tests to pytest here too, and use the tmp_path fixture to handle temporary data and let pytest handle the removal of the temporary files/directories to come around the problem.

In an attempt to make the tests pass on Windows Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

adybbroe · 2022-04-13T21:49:22Z

Okay, so now the tests pass! There is a 0.02% decrease in test coverage. But the Coveralls page is not very friendly - don't see where this decrease sits?
I am off tomorrow, but hopefully it is good enough?

mraspaud

LGTM, much nicer with pytest!

Adam.Dybbroe added 2 commits October 20, 2020 13:08

Merge branch 'master' into fix-osisaf-reader

4599b7a

Add support for reading OSISAF SST GHRSST data

93cfdaa

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

adybbroe self-assigned this Oct 20, 2020

adybbroe added bug enhancement code enhancements, features, improvements hacktoberfest labels Oct 20, 2020

adybbroe requested review from mraspaud, pnuu and simonrp84 October 20, 2020 19:21

adybbroe removed the request for review from simonrp84 October 21, 2020 09:41

Add sensor property to the reader and merge into one yaml file for bo…

1d2e85a

…th SLSTR and OSISAF Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

Adam.Dybbroe added 4 commits March 22, 2022 16:15

Merge branch 'main' into fix-osisaf-reader

957ac50

# Conflicts: # satpy/etc/readers/slstr_l2.yaml # satpy/readers/ghrsst_l2.py Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

Adapt name pattern to handle variable length platform-names ('NOAA-20_')

50aece8

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

Merge branch 'main' into fix-osisaf-reader

a790200

Adding an enhancement to produce an SST image with a color legend

ce2c97f

Color legend originally provided by MET Norway and the OSISAF

adybbroe added the component:readers label Mar 30, 2022

Rename the slstr_l2 test module and extend test coverage a bit

a7e6be0

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

adybbroe marked this pull request as ready for review March 30, 2022 13:44

adybbroe requested a review from djhoese as a code owner March 30, 2022 13:44

adybbroe requested a review from simonrp84 April 4, 2022 12:55

mraspaud requested changes Apr 5, 2022

View reviewed changes

satpy/etc/enhancements/generic.yaml Outdated Show resolved Hide resolved

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

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

satpy/etc/readers/ghrsst_l2.yaml Show resolved Hide resolved

adybbroe and others added 3 commits April 6, 2022 09:21

Update satpy/readers/ghrsst_l2.py

9f1ab58

Follow Python 3 standard for class inherritance Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>

Update list of readers reflecting the change applicable to the GHRSST…

04107ac

… reading Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

Remove SMHI specific SST colormap from the enhancement

cff9d99

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

Skip unpacking the tar archive and let open_data set work on the file…

d5b98b6

… object instead Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

sfinkens reviewed Apr 8, 2022

View reviewed changes

Refactor and skip mocking

5560789

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

sfinkens approved these changes Apr 11, 2022

View reviewed changes

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

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

Adam.Dybbroe added 5 commits April 11, 2022 13:04

Minor refactor

ddb083b

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

Minor refactor

37c72a9

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

Merge branch 'main' into fix-osisaf-reader

2f6a3f5

Remove debug printout

b136887

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

Fix copyright date span

3f2e5c0

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

Skip unittest module and use pytests tmp_path fixture instead

e02abd4

In an attempt to make the tests pass on Windows Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

adybbroe requested a review from mraspaud April 13, 2022 21:49

mraspaud approved these changes Apr 14, 2022

View reviewed changes

mraspaud added 2 commits April 14, 2022 08:06

Refactor constructor

8e79f96

Refactor nc

e3d7dda

mraspaud merged commit 37e77dd into pytroll:main Apr 14, 2022

ameraner mentioned this pull request Dec 9, 2022

slstr_l2.yaml points to deleted slstr_l2.py #2319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix osisaf SST reader #1410

Fix osisaf SST reader #1410

adybbroe commented Oct 20, 2020 •

edited

Loading

adybbroe commented Oct 20, 2020

mraspaud commented Oct 21, 2020

adybbroe commented Oct 21, 2020

mraspaud commented Oct 21, 2020

ghost commented Oct 21, 2020

adybbroe commented Oct 21, 2020

adybbroe commented Mar 25, 2022 •

edited

Loading

adybbroe commented Mar 31, 2022

adybbroe commented Apr 2, 2022

mraspaud left a comment

codecov bot commented Apr 6, 2022 •

edited

Loading

coveralls commented Apr 6, 2022 •

edited

Loading

adybbroe commented Apr 7, 2022 •

edited by mraspaud

Loading

sfinkens left a comment

sfinkens Apr 8, 2022

adybbroe Apr 8, 2022

sfinkens Apr 8, 2022

adybbroe Apr 8, 2022

sfinkens left a comment

adybbroe commented Apr 11, 2022

mraspaud commented Apr 12, 2022

adybbroe commented Apr 13, 2022

mraspaud left a comment

Fix osisaf SST reader #1410

Fix osisaf SST reader #1410

Conversation

adybbroe commented Oct 20, 2020 • edited Loading

adybbroe commented Oct 20, 2020

mraspaud commented Oct 21, 2020

adybbroe commented Oct 21, 2020

mraspaud commented Oct 21, 2020

ghost commented Oct 21, 2020

DeepCode's analysis on #1d2e85 found:

Top issues

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

adybbroe commented Oct 21, 2020

adybbroe commented Mar 25, 2022 • edited Loading

adybbroe commented Mar 31, 2022

adybbroe commented Apr 2, 2022

mraspaud left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2022 • edited Loading

Codecov Report

coveralls commented Apr 6, 2022 • edited Loading

adybbroe commented Apr 7, 2022 • edited by mraspaud Loading

sfinkens left a comment

Choose a reason for hiding this comment

sfinkens Apr 8, 2022

Choose a reason for hiding this comment

adybbroe Apr 8, 2022

Choose a reason for hiding this comment

sfinkens Apr 8, 2022

Choose a reason for hiding this comment

adybbroe Apr 8, 2022

Choose a reason for hiding this comment

sfinkens left a comment

Choose a reason for hiding this comment

adybbroe commented Apr 11, 2022

mraspaud commented Apr 12, 2022

adybbroe commented Apr 13, 2022

mraspaud left a comment

Choose a reason for hiding this comment

adybbroe commented Oct 20, 2020 •

edited

Loading

adybbroe commented Mar 25, 2022 •

edited

Loading

codecov bot commented Apr 6, 2022 •

edited

Loading

coveralls commented Apr 6, 2022 •

edited

Loading

adybbroe commented Apr 7, 2022 •

edited by mraspaud

Loading