FCI L2 CF harmonization #2665

samain-eum · 2023-12-05T07:51:52Z

Added when necessary:

standard names
better long names
fill value
flag_values/meanings
option to import flag_values/meanings from netCDF4 enumerations

codecov · 2023-12-05T08:01:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bca22b4) 95.40% compared to head (8ccae17) 95.89%.
Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2665      +/-   ##
==========================================
+ Coverage   95.40%   95.89%   +0.48%     
==========================================
  Files         371      371              
  Lines       52825    52870      +45     
==========================================
+ Hits        50399    50701     +302     
+ Misses       2426     2169     -257

Flag	Coverage Δ
behaviourtests	`4.15% <0.00%> (-0.01%)`	⬇️
unittests	`95.99% <100.00%> (-0.03%)`	⬇️

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 · 2023-12-05T08:15:15Z

Pull Request Test Coverage Report for Build 7814337362

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

0 of 55 (100.0%) changed or added relevant lines in 2 files are covered.
7 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.009%) to 95.977%

Files with Coverage Reduction	New Missed Lines	%
satpy/readers/fy4_base.py	1	99.32%
satpy/tests/test_readers.py	1	99.36%
satpy/tests/writer_tests/test_cf.py	1	99.14%
satpy/readers/init.py	4	98.65%

Totals
Change from base Build 7472442128:	0.009%
Covered Lines:	50573
Relevant Lines:	52693

💛 - Coveralls

strandgren

see comments inline.

satpy/readers/fci_l2_nc.py

satpy/etc/readers/fci_l2_nc.yaml

strandgren · 2023-12-12T08:54:02Z

satpy/etc/readers/fci_l2_nc.yaml

  cloud_test_cmrt2:
    name: cloud_test_cmrt2
    resolution: 2000
    file_type: nc_fci_test_clm
    file_key: cloud_mask_test_result
-    long_name: cloud_mask_test_cmrt2
    extract_byte: 17
+    flag_values: [0,1]
+    flag_meanings: ['Cloud undetected','Cloud detected']
+    standard_name: status_flag

  cloud_test_cmrt3:
    name: cloud_test_cmrt3
    resolution: 2000
    file_type: nc_fci_test_clm
    file_key: cloud_mask_test_result
-    long_name: cloud_mask_test_cmrt3
    extract_byte: 18
+    flag_values: [0,1]
+    flag_meanings: ['Cloud undetected','Cloud detected']
+    standard_name: status_flag

  cloud_test_cmrt4:
    name: cloud_test_cmrt4
    resolution: 2000
    file_type: nc_fci_test_clm
    file_key: cloud_mask_test_result
-    long_name: cloud_mask_test_cmrt4
    extract_byte: 19
+    flag_values: [0,1]
+    flag_meanings: ['Cloud undetected','Cloud detected']
+    standard_name: status_flag

  cloud_test_cmrt5:
    name: cloud_test_cmrt5
    resolution: 2000
    file_type: nc_fci_test_clm
    file_key: cloud_mask_test_result
-    long_name: cloud_mask_test_cmrt5
    extract_byte: 20
+    flag_values: [0,1]
+    flag_meanings: ['Cloud undetected','Cloud detected']
+    standard_name: status_flag


Use flag_meanings from FCIL2FS. Those are restoration tests.

For better clarity I would propose:

For cloud_test_cmrt2-4: flag_meanings: ['Clear unchanged', 'Cloud detected (restored from clear sky)']

For cloud_test_cmrt5: flag_meanings: ['Clear sky restored', 'Cloud unchanged'] for better clarity.

Seems like this was also partly unfixed with commit: 1c02d1c

At least cloud_test_cmrt3 should have flag_meanings: ['Clear unchanged', 'Cloud detected (restored from clear sky)'] which it does no longer have.

Fixed cloud_test_cmrt3

satpy/etc/readers/fci_l2_nc.yaml

strandgren · 2024-01-05T13:38:35Z

satpy/etc/readers/fci_l2_nc.yaml


  n_acc:
    name: n_acc
    resolution: 1000
    file_type: nc_fci_crm
    file_key: number_of_accumulations
-    long_name: number_of_accumulations
+    standard_name: number_of_accumulations

  historical_data:


historical_data is a categorical dataset and we should be able to read flag_values and flag_meanings information from the enum.

Added enum import flag

strandgren · 2024-01-05T13:42:14Z

satpy/etc/readers/fci_l2_nc.yaml


  retrieved_cloud_optical_thickness_upper_layer:
    name: retrieved_cloud_optical_thickness_upper_layer
    resolution: 2000
    file_type: nc_fci_oca
    file_key: retrieved_cloud_optical_thickness
    layer: 0
-    long_name: cloud_optical_depth


Here I would add the long_name from the FCIL2FS but with the addition "(upper layer)" at the end

appended "for upper/lower layer" to the long names

strandgren · 2024-01-05T13:43:09Z

satpy/etc/readers/fci_l2_nc.yaml

+    file_type: nc_fci_oca
+    file_key: retrieval_error_cloud_optical_thickness
+    layer: 0
+    standard_name: atmosphere_optical_thickness_due_to_cloud standard_error


Here I would add the long_name from the FCIL2FS but with the addition "(upper layer)" at the end

appended "for upper/lower layer" to the long names

strandgren · 2024-01-05T13:43:24Z

satpy/etc/readers/fci_l2_nc.yaml


  retrieved_cloud_optical_thickness_lower_layer:
    name: retrieved_cloud_optical_thickness_lower_layer
    resolution: 2000
    file_type: nc_fci_oca
    file_key: retrieved_cloud_optical_thickness
    layer: 1
-    long_name: cloud_optical_depth


Here I would add the long_name from the FCIL2FS but with the addition "(lower layer)" at the end

appended "for upper/lower layer" to the long names

strandgren · 2024-01-05T13:43:35Z

satpy/etc/readers/fci_l2_nc.yaml

+    file_type: nc_fci_oca
+    file_key: retrieval_error_cloud_optical_thickness
+    layer: 1
+    standard_name: atmosphere_optical_thickness_due_to_cloud standard_error


Here I would add the long_name from the FCIL2FS but with the addition "(lower layer)" at the end

appended "for upper/lower layer" to the long names

strandgren · 2024-01-05T13:44:28Z

satpy/etc/readers/fci_l2_nc.yaml


  retrieved_cloud_particle_effective_radius:
-    name: retrieved_cloud_particle_effective_radius
+    name: retrieved_cloud_particle_effective_radius_upper_layer


remove "_upper_layer" since this a 2D dataset, which is independent of the retrieved layers

strandgren · 2024-01-05T13:45:05Z

satpy/etc/readers/fci_l2_nc.yaml

-  retrieved_cloud_top_temperature:
-    name: retrieved_cloud_top_temperature
+  retrieval_error_cloud_particle_effective_radius:
+    name: retrieval_error_cloud_particle_effective_radius_upper_layer


remove "_upper_layer" since this a 2D dataset, which is independent of the retrieved layers

strandgren · 2024-01-05T13:45:28Z

satpy/etc/readers/fci_l2_nc.yaml

    resolution: 2000
    file_type: nc_fci_oca
    file_key: retrieved_cloud_particle_effective_radius
-    standard_name: effective_radius_of_cloud_condensed_water_particles_at_cloud_top
+    standard_name: effective_radius_of_cloud_particles


effective_radius_of_cloud_particles --> effective_radius_of_cloud_particles_at_cloud_top

strandgren · 2024-01-05T13:45:40Z

satpy/etc/readers/fci_l2_nc.yaml

-    file_key: retrieved_cloud_top_temperature
-    standard_name: air_temperature_at_cloud_top
+    file_key: retrieval_error_cloud_particle_effective_radius
+    standard_name: effective_radius_of_cloud_particles standard_error


effective_radius_of_cloud_particles --> effective_radius_of_cloud_particles_at_cloud_top

appended "_at_cloud_top" to both retrieved effective radius and its error

strandgren · 2024-01-05T13:46:11Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -340,6 +401,14 @@ datasets:
    layer: 0
    standard_name: air_pressure_at_cloud_top


Here I would add the long_name from the FCIL2FS but with the addition "(upper layer)" at the end

appended "for upper/lower layer" to the long names

strandgren · 2024-01-05T13:46:31Z

satpy/etc/readers/fci_l2_nc.yaml

+    file_type: nc_fci_oca
+    file_key: retrieval_error_cloud_top_pressure
+    layer: 0
+    standard_name: air_pressure_at_cloud_top standard_error


Here I would add the long_name from the FCIL2FS but with the addition "(upper layer)" at the end

appended "for upper/lower layer" to the long names

strandgren · 2024-01-05T13:46:42Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -348,163 +417,147 @@ datasets:
    layer: 1
    standard_name: air_pressure_at_cloud_top


Here I would add the long_name from the FCIL2FS but with the addition "(lower layer)" at the end

appended "for upper/lower layer" to the long names

strandgren · 2024-01-05T13:48:24Z

satpy/etc/readers/fci_l2_nc.yaml

    layer: 1
-    long_name: cloud_optical_depth
+    standard_name: air_pressure_at_cloud_top standard_erro


Here I would add the long_name from the FCIL2FS but with the addition "(lower layer)" at the end

appended "for upper/lower layer" to the long names

strandgren · 2024-01-05T13:48:41Z

satpy/etc/readers/fci_l2_nc.yaml

    layer: 1
-    long_name: cloud_optical_depth
+    standard_name: air_pressure_at_cloud_top standard_erro


standard_erro --> standard_error

strandgren · 2024-01-05T13:48:55Z

satpy/etc/readers/fci_l2_nc.yaml

-  retrieval_error_cloud_top_pressure_upper_layer:
-    name: retrieval_error_cloud_top_pressure_upper_layer
+  retrieved_cloud_top_temperature:
+    name: retrieved_cloud_top_temperature_upper_layer


remove "_upper_layer" since this a 2D dataset, which is independent of the retrieved layers

strandgren · 2024-01-05T13:49:11Z

satpy/etc/readers/fci_l2_nc.yaml

-  retrieval_error_cloud_top_pressure_lower_layer:
-    name: retrieval_error_cloud_top_pressure_lower_layer
+  retrieved_cloud_top_height:
+    name: retrieved_cloud_top_height_upper_layer


remove "_upper_layer" since this a 2D dataset, which is independent of the retrieved layers

strandgren · 2024-01-05T14:20:48Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -1250,7 +1349,9 @@ datasets:
    resolution: 32000
    file_type: nc_fci_asr
    file_key: bt_std
-    long_name: brightness_temperature_standard_deviation_in_segment
+    long_name: TOA Brightess Temperature standard deviation


standard deviation --> segment standard deviation

strandgren · 2024-01-05T14:22:01Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -1260,7 +1361,9 @@ datasets:
    resolution: 32000
    file_type: nc_fci_asr
    file_key: radiance_max
-    long_name: maximum_radiance_in_segment
+    long_name: TOA max Radiance


align with naming above: TOA radiance segment max

strandgren · 2024-01-05T14:23:17Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -1270,7 +1373,9 @@ datasets:
    resolution: 32000
    file_type: nc_fci_asr
    file_key: radiance_mean
-    long_name: mean_radiance_in_segment
+    long_name: TOA mean Radiance


align with naming above: TOA radiance segment mean

strandgren · 2024-01-05T14:23:27Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -1280,7 +1385,9 @@ datasets:
    resolution: 32000
    file_type: nc_fci_asr
    file_key: radiance_min
-    long_name: minimum_radiance_in_segment
+    long_name: TOA min Radiance


align with naming above: TOA radiance segment min

strandgren · 2024-01-05T14:23:40Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -1290,7 +1397,9 @@ datasets:
    resolution: 32000
    file_type: nc_fci_asr
    file_key: radiance_std
-    long_name: radiance_standard_deviation_in_segment
+    long_name: TOA Outgoing Radiance standard deviation


align with naming above: TOA radiance segment standard deviation

strandgren · 2024-01-05T14:24:16Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -1330,7 +1445,9 @@ datasets:
    resolution: 32000
    file_type: nc_fci_asr
    file_key: reflectance_std
-    long_name: reflectance_standard_deviation_in_segment
+    long_name: TOA Bidirectional Reflectance standard deviation


align with naming above: TOA Bidirectional Reflectance segment standard deviation

strandgren · 2024-01-05T14:27:32Z

satpy/etc/readers/fci_l2_nc.yaml

    coordinates:
    - longitude
    - latitude

  quality_radiance:
    name: quality_radiance
    resolution: 32000
+    wavelength: []


please remove.

fixed
I had a few of those because if I don't include that line in the script, the writing of the full bloc is messed up somehow. So I had to remove it manually afterward.

strandgren · 2024-01-05T14:30:48Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -1373,7 +1494,7 @@ datasets:
    resolution: 32000
    file_type: nc_fci_asr
    file_key: land_pixel_percent
-    long_name: land_pixel_percentage_in_segment
+    standard_name: land_area_fraction


also add units: %, set to none in netcdf file.

strandgren · 2024-01-05T14:30:53Z

satpy/etc/readers/fci_l2_nc.yaml

@@ -1383,7 +1504,7 @@ datasets:
    resolution: 32000
    file_type: nc_fci_asr
    file_key: water_pixel_percent
-    long_name: water_pixel_percentage_in_segment
+    standard_name: water_area_fraction


also add units: %, set to none in netcdf file.

sjoro

a few comments:

use of segmented-keyword in _set_attributes-method and then checking this (lines 84-89 in fci_l2_nc.py) and renaming dimensions accordingly should be simplified. as the dimension names are fixed they should be set in respective classes as class (or instant) variables , e.g.:

class FciL2NCFileHandler(FciL2CommonFunctions, BaseFileHandler):
    """Reader class for FCI L2 products in NetCDF4 format."""

    xdim, ydim = "number_of_columns", "number_of_rows"

    def __init__(self, filename, filename_info, filetype_info, with_area_definition=True):
        """Open the NetCDF file with xarray and prepare for dataset reading."""
    ...

then the method could be simplified as

    def _set_attributes(self, variable, dataset_info):
        """Set dataset attributes."""
        if dataset_info["file_key"] not in ["product_quality", "product_completeness", "product_timeliness"]:
            variable = variable.rename({self.ydim: "y", self.xdim: "x"})
        ...

fci_l2_nc.yaml -file has a lot of duplicate datasets. for example, quality_illumination is defined for clm as quality_illumination_clm, the same dataset is defined in ct-product as quality_illumination_ct. this dataset should only be defined once and use file_type to assign the dataset to respective products:

  quality_illumination:
    name: quality_illumination
    resolution: 2000
    file_type: [nc_fci_clm, nc_fci_ct]
    file_key: quality_illumination
    long_name: illumination_classification
    standard_name: status_flag
    fill_value: -127
    import_enum_information: True

multiple similar cases can be found, e.g. quality_nwp_parameters, quality_MTG_parameters, quality_overall_processing, etc...

a nitpick comment on the order of the keys for datasets... i would prefer to see the order name, long_name, standard_name, resolution, file_type, and then the other keys... please see seviri_l2_grib.yaml as an example. similar ordering was done there.
last nitpick... i would rename file_key to nc_key for clarity. maybe this is just me but reading dataset_info["file_key"] confuses me everytime. i know file_key is used in other readers, too. i find it equally bad there too :D

thanks for working on this olivier! much appreciated to get this reader sorted out!

sjoro · 2024-01-12T08:43:35Z

satpy/readers/fci_l2_nc.py

        variable.attrs.update(dataset_info)
        variable.attrs.update(self._get_global_attributes())

+        import_enum_information = dataset_info.get("import_enum_information", False)
+        if (import_enum_information):


in order to reduce complexity, this block, i.e. adding flag_values and flag_meanings as attributes should be a separate method, e.g. _set_flag_values_and_meanings

sjoro · 2024-01-12T09:12:27Z

satpy/etc/readers/fci_l2_nc.yaml

-    long_name: quality_index
+    standard_name: status_flag
+    fill_value: -127
+    import_enum_information: True

  quality_MTG_parameters_clm:


dataset names are typically all lower case, please rename to quality_mtg_parameters (also remove _clm, see other comment)

…ion to a variable

…names.

…nto fci_l2_CF_hamonization

strandgren · 2024-02-19T11:41:42Z

This PR is now ready for review. The main modifications are related to CF harmonization of the dataset attributes, so no real change in the reader code itself, except for improved handing on the unit in order to comply with CF as well as the extraction of the flag_values and flag_meanings from the netcdf file if available.

For the quality assessment datasets the dataset names have been simplified thanks to the use of list of applicable file_types instead (e.g. product_quality_clm -> product_quality). However, since neither test nor real FCI L2 data have been released, this should not be an issue and users should benefit from the harmonized naming convention.

mraspaud

Lgtm, thanks for adding the new tests!

strandgren assigned samain-eum Dec 11, 2023

strandgren reviewed Dec 12, 2023

View reviewed changes

samain-eum force-pushed the fci_l2_CF_hamonization branch from 7a97a03 to 1c02d1c Compare December 15, 2023 13:28

strandgren reviewed Jan 5, 2024

View reviewed changes

strandgren added 2 commits January 11, 2024 09:18

Add tests for unit extraction and assignment

176a66c

Fix failing test

d75b9e5

samain-eum force-pushed the fci_l2_CF_hamonization branch from 43140b3 to d75b9e5 Compare January 11, 2024 08:21

samain-eum marked this pull request as ready for review January 11, 2024 08:36

samain-eum requested review from djhoese and mraspaud as code owners January 11, 2024 08:36

sjoro requested changes Jan 12, 2024

View reviewed changes

sjoro added component:readers cleanup Code cleanup but otherwise no change in functionality labels Jan 12, 2024

sjoro added 2 commits January 12, 2024 14:27

Change order of yaml-key.

7afbf0e

Refactor order of yaml-keys in fci_l2_nc.yaml.

fa7ff05

samain-eum marked this pull request as draft January 15, 2024 12:52

samain-eum added 5 commits January 15, 2024 16:27

Harmonize key order for ASR

817d8be

changed file_key to nc_key

093921c

created separate method to add flag values and meanings from enumerat…

b3f996c

…ion to a variable

add missing units. Remove radiance_mean_<category>_<channel>

40e6ad5

made common parameters generic

c8901a1

samain-eum marked this pull request as ready for review January 29, 2024 10:27

strandgren and others added 8 commits January 29, 2024 12:09

make all dataset names lower-case and remove _<product> from dataset …

c394307

…names.

Harmonize dataset names.

2afb5ed

Use flag_values from enum dict.

80f0b2d

Fix code style.

6d888be

Use swap_dims instead of rename to rename dimensions.

82750f1

Fix typo.

9cf9397

Make dataset name lower-case.

5faf2e3

Merge remote-tracking branch 'osamain_satpy/fci_l2_CF_hamonization' i…

8ccae17

…nto fci_l2_CF_hamonization

mraspaud approved these changes Feb 20, 2024

View reviewed changes

mraspaud merged commit 6c19e44 into pytroll:main Feb 20, 2024
18 of 19 checks passed

strandgren mentioned this pull request Feb 20, 2024

Standardize dataset information for SEVIRI and FCI L2 products #1921

Closed

		@@ -340,6 +401,14 @@ datasets:
		layer: 0
		standard_name: air_pressure_at_cloud_top

		@@ -348,163 +417,147 @@ datasets:
		layer: 1
		standard_name: air_pressure_at_cloud_top

FCI L2 CF harmonization #2665

FCI L2 CF harmonization #2665

Conversation

samain-eum commented Dec 5, 2023

codecov bot commented Dec 5, 2023 • edited Loading

Codecov Report

coveralls commented Dec 5, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7814337362

Warning: This coverage report may be inaccurate.

💛 - Coveralls

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

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

strandgren Jan 5, 2024 • edited Loading

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoro Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

strandgren commented Feb 19, 2024

mraspaud left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2023 •

edited

Loading

coveralls commented Dec 5, 2023 •

edited

Loading

strandgren Jan 5, 2024 •

edited

Loading

sjoro left a comment •

edited

Loading

sjoro Jan 12, 2024 •

edited

Loading