Changes to Eumetsat L2 BUFR reader #2603

samain-eum · 2023-10-19T08:40:02Z

This PL carries the following feature changes:

Adapt the EUM BUFR reader to make it generic and no longer SEVIRI speficic (hence the name change)
Add functionality and configuration yaml file for the FCI BUFR products
Include a fix / workaround for the case a variable within a BUFR message is all set to fill values, which results in an unexpected array size as returned by eccodes

…eader) and modify seviri_l2_bufr.yaml accordingly.

… script accordingly.

…nstead of MET08-11).

…nd adapt code to handle all variables being returned as arrays

strandgren

Good job, see comments in-line.

satpy/readers/eum_l2_bufr.py

satpy/tests/reader_tests/test_eum_l2_bufr.py

strandgren

see some more comments in-line.

satpy/tests/reader_tests/test_eum_l2_bufr.py

strandgren · 2023-12-11T12:49:17Z

satpy/etc/readers/fci_l2_bufr.yaml

+  latitude:
+    name: latitude
+    key: '#1#latitude'
+    resolution: [32000]


Add resolution of AMV product

strandgren · 2023-12-11T12:49:21Z

satpy/etc/readers/fci_l2_bufr.yaml

+  longitude:
+    name: longitude
+    key: '#1#longitude'
+    resolution: [32000]


Add resolution of AMV product

satpy/etc/readers/fci_l2_bufr.yaml

satpy/readers/eum_l2_bufr.py

Harmonize key order for AMV Fix single quote style complains

strandgren

Thanks @samain-eum, we're getting close now :) Some more comments in-line, There are also a few minor open comments from the last review, see here: #2603 (review) - only the ones related to renaming SEVIRI --> EUMETSAT, the rest should be fixed, or tracked otherwise.

I also noticed that the units of the SEVIRI GII datasets are not set correctly, could you maybe align this with the latest in the fci_l2_nc reader? i.e.

ki: deg Celcius
ko: deg Celcius
li: deg Celcius
mb: deg Celcius
lpw1: kg/m2 (mm is also correct, but kg/m2 is in line with fci_l2_nc and also seems more inline with CF)
lpw2: kg/m2 
lpw3: kg/m2
tpw1: kg/m2

satpy/etc/readers/fci_l2_bufr.yaml

satpy/readers/eum_l2_bufr.py

samain-eum · 2024-04-12T14:19:11Z

@mraspaud : We have a question on what do for the resolution of wind products
For the various dedicated variables, we chose to set it to "none", as a spatial resolution doesn't make sense for a wind vector. It shows up as such in the variable attributes imported into satpy.
However we have also "common" variables, i.e. latitude and longitude, where the resolution key is defined as an array, with one element per product in the file_type key. It doesn't seem possible to mix different types for the resolution (it's an array, not a list).
The solutions I have found are either:

Put a placeholder value for the AMV resolution (something that won't break anything, i.e. 0 or 1 perhaps)
Create an other kind of latitude / longitude bloc entry for resolution-less data. Same name, just different file types allocated, and resolution: ["none"].

Do you have any preference of advice?

strandgren · 2024-05-27T10:14:06Z

@mraspaud : We have a question on what do for the resolution of wind products For the various dedicated variables, we chose to set it to "none", as a spatial resolution doesn't make sense for a wind vector. It shows up as such in the variable attributes imported into satpy. However we have also "common" variables, i.e. latitude and longitude, where the resolution key is defined as an array, with one element per product in the file_type key. It doesn't seem possible to mix different types for the resolution (it's an array, not a list). The solutions I have found are either:

Put a placeholder value for the AMV resolution (something that won't break anything, i.e. 0 or 1 perhaps)

Create an other kind of latitude / longitude bloc entry for resolution-less data. Same name, just different file types allocated, and resolution: ["none"].

Do you have any preference of advice?

To add to this: what is actually the idea of having a list of resolutions for a datasets resolution variable for a dataset in the reader yaml-file? How would satpy know what resolution is the correct one when loading e.g. latitude if there is a list of three values? I first thought there was a relationship between the file_type list and the resolution list, but since equal lengths of these lists/arrays is not needed, that cannot be the case.

If you know the logic behind this @mraspaud or could point to the relevant code that would help us understand better what the best approach is for our use case.

samain-eum · 2024-05-27T12:01:04Z

@strandgren Actually my idea of separate different lat/lon blocs for resolution-less products doesn't work. It seems satpy overloads the dataset info with the last bloc found in the yaml file and the same name. So it breaks when trying to load lat/lon for ASR for example. So I think I will instead use a dummy value of 0 in the yaml file, and replace it by 'none' when creating the attributes.

As for your question above, the resolution is currently linked to the file pattern itself, so we can't manage datasets with different resolutions within one file.

mraspaud · 2024-05-27T13:06:48Z

That's a tough question!
The resolution is indeed quite central in satpy, and I don't believe None is an option.

But just for curiosity, why doesn't resolution make sense for wind data? Is it like a continuous field? or is it motion vector derived from pixels at a certain resolution? (in which case maybe the original pixel resolution makes a little sense?)

Otherwise, you can always define custom data identification keys for your reader, like in here: https://github.com/pytroll/satpy/blob/main/satpy/etc/readers/sgli_l1b.yaml#L13-L31
And from there, maybe you can have a resolution-less one? I'm quite sure it will break somewhere, but maybe worth a try?

samain-eum · 2024-05-28T13:42:59Z

Hi @mraspaud
Wind vectors are produced by finding cloud features that are moving during successive images and a product is a "list" of winds with their lat/lon information, not a gridded product. So yes, resolution don't really make sense.
I'm aware that a None (i.e. the python reserved type) may cause issues, that's why I opted for a "none" string instead. In the scope of our unit tests, it seems to work fine now. It just need dedicated lat/lon coordinates variables with the same value for the resolution.
Maybe it's worth noting that for those product, the 'with_area_definition' flag when creating a scene instance is ignored, so we make no attempt at building an area definition (which requires a resolution). We are not aware of anything else that could break satpy.

This is how the definitions look like now for AMV:

  latitude_amv:
    name: latitude_amv
    key: '#1#latitude'
    resolution: none
    file_type: fci_l2_bufr_amv
    standard_name: latitude
    units: degree_north
    fill_value: -1.e+100
 
  longitude_amv:
    name: longitude_amv
    key: '#1#longitude'
    resolution: none
    file_type: fci_l2_bufr_amv
    standard_name: longitude
    units: degree_east
    fill_value: -1.e+100
 
  pressure:
    name: pressure
    long_name: Pressure of AMV feature
    standard_name: air_pressure_at_wind_level
    resolution: none
    file_type: fci_l2_bufr_amv
    key: '#1#pressure'
    units: Pa
    fill_value: -1.0e+100
    coordinates:
      - longitude_amv
      - latitude_amv

…_size attributes of returned dataset array to None

…r_reader

codecov · 2024-06-14T08:17:56Z

Codecov Report

Attention: Patch coverage is 96.15385% with 8 lines in your changes missing coverage. Please review.

Project coverage is 95.97%. Comparing base (ab55c4a) to head (3b5c482).
Report is 258 commits behind head on main.

Files	Patch %	Lines
satpy/readers/eum_l2_bufr.py	87.30%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2603      +/-   ##
==========================================
+ Coverage   95.91%   95.97%   +0.05%     
==========================================
  Files         366      366              
  Lines       53504    53804     +300     
==========================================
+ Hits        51321    51639     +318     
+ Misses       2183     2165      -18

Flag	Coverage Δ
behaviourtests	`4.03% <0.00%> (-0.01%)`	⬇️
unittests	`96.07% <96.15%> (+0.05%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coveralls · 2024-06-14T08:38:41Z

Pull Request Test Coverage Report for Build 9512734622

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

197 of 210 (93.81%) changed or added relevant lines in 2 files are covered.
42 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.02%) to 96.023%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/tests/reader_tests/test_eum_l2_bufr.py	142	146	97.26%
satpy/readers/eum_l2_bufr.py	55	64	85.94%

Files with Coverage Reduction	New Missed Lines	%
satpy/resample.py	42	88.74%

Totals
Change from base Build 9418285216:	-0.02%
Covered Lines:	51591
Relevant Lines:	53728

💛 - Coveralls

strandgren · 2024-06-14T12:53:57Z

This one is now ready for review. In the end we decided to not specify a resolution in the yaml-files for the AMV data. We also kept a single definition of the latitude and longitude datasets that we use also for the AMV datasets. For these latitude and longitude datasets we have also assigned as many resolutions as file_types in orders to have a clear mapping of what the resolution is for a given file_type. For the AMV file_type we have set the latitude and longitude resolution to -1 since the list of resolutions does not support different data types (i.e. NoneType). Beyond the unit tests we have also tested this PR with real data and it appears to work well. See example below for MSG AMV data, adding the channel_id dataset, also allows to separate the AMV by channel (left: all channels, right: IR10.8 channel):

coveralls · 2024-06-14T13:23:36Z

Pull Request Test Coverage Report for Build 9516424732

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

201 of 209 (96.17%) changed or added relevant lines in 2 files are covered.
42 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.008%) to 96.032%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/eum_l2_bufr.py	55	63	87.3%

Files with Coverage Reduction	New Missed Lines	%
satpy/resample.py	42	88.74%

Totals
Change from base Build 9418285216:	-0.008%
Covered Lines:	51595
Relevant Lines:	53727

💛 - Coveralls

mraspaud

Thanks for adding this reader! I would be in favor of writing °C instead of \u00B0C for readability, otherwise it looks good to me!

mraspaud · 2024-07-19T10:12:33Z

satpy/etc/readers/seviri_l2_bufr.yaml

@@ -1081,7 +1081,7 @@ datasets:
    coordinates:
       - longitude
       - latitude
-    units: ""
+    units: "\u00B0C"


Suggested change

units: "\u00B0C"

units: "°C"

Updated as suggested

mraspaud · 2024-07-19T10:12:48Z

satpy/etc/readers/seviri_l2_bufr.yaml

@@ -1094,7 +1094,7 @@ datasets:
    coordinates:
       - longitude
       - latitude
-    units: ""
+    units: "\u00B0C"


Suggested change

units: "\u00B0C"

units: "°C"

Updated as suggested

mraspaud · 2024-07-19T10:13:02Z

satpy/etc/readers/seviri_l2_bufr.yaml

@@ -1107,7 +1107,7 @@ datasets:
    coordinates:
       - longitude
       - latitude
-    units: ""
+    units: "\u00B0C"


Suggested change

units: "\u00B0C"

units: "°C"

Updated as suggested

mraspaud · 2024-07-19T10:13:17Z

satpy/etc/readers/seviri_l2_bufr.yaml

@@ -1159,7 +1159,7 @@ datasets:
    coordinates:
       - longitude
       - latitude
-    units: ""
+    units: "\u00B0C"


Suggested change

units: "\u00B0C"

units: "°C"

Updated as suggested

coveralls · 2024-07-23T08:41:08Z

Pull Request Test Coverage Report for Build 10055342407

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

201 of 209 (96.17%) changed or added relevant lines in 2 files are covered.
151 unchanged lines in 15 files lost coverage.
Overall coverage increased (+0.04%) to 96.075%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/eum_l2_bufr.py	55	63	87.3%

Files with Coverage Reduction	New Missed Lines	%
satpy/tests/reader_tests/gms/test_gms5_vissr_navigation.py	1	99.3%
satpy/readers/generic_image.py	1	97.7%
satpy/enhancements/init.py	1	99.07%
satpy/readers/init.py	2	98.68%
satpy/readers/viirs_edr.py	2	98.92%
satpy/tests/enhancement_tests/test_enhancements.py	2	99.44%
satpy/readers/hrit_jma.py	2	98.64%
satpy/tests/reader_tests/test_ami_l1b.py	3	98.17%
satpy/readers/ami_l1b.py	4	97.32%
satpy/readers/hdf5_utils.py	5	92.77%

Totals
Change from base Build 9418285216:	0.04%
Covered Lines:	51868
Relevant Lines:	53987

💛 - Coveralls

strandgren and others added 11 commits October 18, 2023 16:56

Add new EUMETSAT L2 BUFR product reader (based on seviri_l2_bufr.py r…

4a47cf2

…eader) and modify seviri_l2_bufr.yaml accordingly.

Remove seviri_l2_bufr reader (replaced by eum_l2_bufr) and adapt test…

bc5f76d

… script accordingly.

Modify test to adhere to new MSG platform naming convention (MSG1-4 i…

b5f0960

…nstead of MET08-11).

Initiate yaml file for fci_l2_bufr reader.

ffb9539

Add config for FCI BUFR products

6882564

fixed issue with unit format

6382cda

Add positional information (#) to all keys. Change ASR long names

b5e292e

fixes to FCI BUFR

7a0fabc

Fixes to FCI BUFR reader and configuration

ff506fd

Use ECCODES_BUFR_MULTI_ELEMENT_CONSTANT_ARRAYS environment variable a…

2296fba

…nd adapt code to handle all variables being returned as arrays

Change to make the test generic for all EUM L2 BUFR products

59604d5

strandgren reviewed Nov 24, 2023

View reviewed changes

Changes following first MR review

a5fadce

strandgren assigned samain-eum Dec 11, 2023

strandgren reviewed Dec 11, 2023

View reviewed changes

samain-eum added 2 commits January 11, 2024 14:48

restore resolution key in yaml files

4e1257b

Use Null for AMV resolution

bc854a3

strandgren mentioned this pull request Jan 11, 2024

Add combined GRIB reader for both SEVIRI and FCI L2 products #2717

Merged

3 tasks

samain-eum added 8 commits January 22, 2024 15:41

change key order

9cd0a85

rename files to make them generic readers / unit test

05a7fb8

Updated EUM BUFR readers and yaml files

5566d23

Add missing entries for AMV

ef06cc4

Add missing entries for AMV. Harmonized order of keys

83363f1

delete blank line

f62ae00

merge with upstream

9130c71

Add new variables to AMV products

898d417

Harmonize key order for AMV Fix single quote style complains

strandgren reviewed Apr 3, 2024

View reviewed changes

samain-eum added 3 commits April 4, 2024 16:34

Remove some double spaces

f2edcb3

replace SEVIRI by EUMETSAT in some places

2445742

fixes for units / resolution

ecfdfbf

fix reader type for AMV files pytroll#2

ecbec24

handling of resolution-less datasets (AMV)

d78f9c2

samain-eum and others added 7 commits May 29, 2024 11:50

Actually removes resolution from AMV entries and set resolution / seg…

847fc06

…_size attributes of returned dataset array to None

slight improvements to resolution handling

20825fa

Merge remote-tracking branch 'upstream/main' into feature_add_eum_buf…

d80df41

…r_reader

change to random generator

a70db2a

Add and modify SEVIRI GII dataset units.

5ac1df8

File clean up.

191daec

Remove blank whitespace

825c146

strandgren marked this pull request as ready for review June 13, 2024 15:46

strandgren requested review from djhoese and mraspaud as code owners June 13, 2024 15:46

Move setting of BUFR ENV variable to init and delete when finished.

653e6a4

Refactor tests for easier maintainence.

2f9dbe9

mraspaud approved these changes Jul 19, 2024

View reviewed changes

samain-eum added 2 commits July 23, 2024 10:16

Fix unit for the ki/li/ko GII indices

01dd7a4

Fix unit for the mb GII index

3b5c482

mraspaud merged commit 980df17 into pytroll:main Jul 26, 2024
18 checks passed

mraspaud added enhancement code enhancements, features, improvements component:readers labels Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to Eumetsat L2 BUFR reader #2603

Changes to Eumetsat L2 BUFR reader #2603

samain-eum commented Oct 19, 2023

strandgren left a comment

strandgren left a comment

strandgren Dec 11, 2023

strandgren Dec 11, 2023

strandgren left a comment

samain-eum commented Apr 12, 2024

strandgren commented May 27, 2024

samain-eum commented May 27, 2024

mraspaud commented May 27, 2024

samain-eum commented May 28, 2024

codecov bot commented Jun 14, 2024 •

edited

Loading

coveralls commented Jun 14, 2024 •

edited

Loading

strandgren commented Jun 14, 2024

coveralls commented Jun 14, 2024 •

edited

Loading

mraspaud left a comment

mraspaud Jul 19, 2024

samain-eum Jul 23, 2024

mraspaud Jul 19, 2024

samain-eum Jul 23, 2024

mraspaud Jul 19, 2024

samain-eum Jul 23, 2024

mraspaud Jul 19, 2024

samain-eum Jul 23, 2024

coveralls commented Jul 23, 2024

Changes to Eumetsat L2 BUFR reader #2603

Changes to Eumetsat L2 BUFR reader #2603

Conversation

samain-eum commented Oct 19, 2023

strandgren left a comment

Choose a reason for hiding this comment

strandgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strandgren left a comment

Choose a reason for hiding this comment

samain-eum commented Apr 12, 2024

strandgren commented May 27, 2024

samain-eum commented May 27, 2024

mraspaud commented May 27, 2024

samain-eum commented May 28, 2024

codecov bot commented Jun 14, 2024 • edited Loading

Codecov Report

coveralls commented Jun 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9512734622

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

strandgren commented Jun 14, 2024

coveralls commented Jun 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9516424732

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

mraspaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 23, 2024

Pull Request Test Coverage Report for Build 10055342407

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

codecov bot commented Jun 14, 2024 •

edited

Loading

coveralls commented Jun 14, 2024 •

edited

Loading

coveralls commented Jun 14, 2024 •

edited

Loading