Fix FCI L1c reader for African products #2866

ameraner · 2024-08-01T11:03:36Z

This PR fixes the fci_l1c_nc reader for the African products, after #2843 introduced/exposed a bug.

#2843 changes the filename pattern from erraneous_count_in_repeat_cycle to count_in_repeat_cycle. With this, this line:

satpy/satpy/readers/yaml_reader.py

Line 1192 in bf0d9cd

fh.filename_info["segment"] = fh.filename_info.get("count_in_repeat_cycle", 1)

does not use the get default 1 value anymore, but uses the actual value coming from the actual filepattern, which is 0. This then activates the padding mechanism, which then fails due to fci_l1c_nc.FCIL1cNCFileHandler.get_segment_position_info not supporting the African products (as it shouldn't need to).
The test that was specifically implemented to check that get_segment_position_info is not called, and that should have failed in the #2843 PR, was faulty due to an extra unnecessary mocking that was invalidating the test.

This PR

fixes the root cause of the unwanted get_segment_position_info execution by assigning the chunk number 1 to the African products, so that the padding does not get activated (1 chunk is present and one is expected).
fixes the test to actually do what it should (the new test would have failed with feat: Enable to read for the Q4 coverage and the IQTI files for the fci l1c data #2843, and it does not now due to the reader fix).

On top

it fixes the filename pattern for the watervapour channels, that instead of WV use IR in the filename
it splits the VIS0.6 channel into two separate filetypes for the 1km and 3km version, so that they can be loaded together and satpy will automatically pick the 1km file if available, as done for the normal files.
on a side, it cleans up the test and reader file for many new style issues related to formatting, typos, static methods declarations, etc. (even though pre-commit was passing)
it also updates and clarifies some more docstrings, variable names, etc.

Sorry for the noise of the reformatting, to make the review easier I have marked with comments the actual relevant code changes.

Tests added
Fully documented

…o enable the nominal padding mechanism (which should not be actually activated anyway)

…ducts is modified correctly

ameraner · 2024-08-01T11:04:59Z

satpy/readers/fci_l1c_nc.py

+        if self.filename_info["coverage"] == "AF":
+            # change number of chunk from 0 to 1 so that the padding is not activated (chunk 1 is present and only 1
+            # chunk is expected), as the African dissemination products come in one file per full disk.
+            self.filename_info["count_in_repeat_cycle"] = 1


this sorts out the padding activation bug for the African products

ameraner · 2024-08-01T11:06:05Z

satpy/tests/reader_tests/test_fci_l1c_nc.py

@@ -897,24 +921,30 @@ def test_get_segment_position_info(self, reader_configs, fh_param, expected_pos_
            segpos_info = filetype_handler.get_segment_position_info()
            assert segpos_info == expected_pos_info

-    @mock.patch("satpy.readers.yaml_reader.GEOVariableSegmentYAMLReader")


this mocking was preventing the test to actually test the execution

ameraner · 2024-08-01T11:07:53Z

satpy/tests/reader_tests/test_fci_l1c_nc.py

+            try:
+                # attempt to load the channel
+                reader.load([channel])
+            except KeyError:
+                # If get_segment_position_info is called, the code will fail with a KeyError because of the mocking.
+                # So we catch the error here for now, but the test will still fail with the following assert_not_called
+                pass
            gspi.assert_not_called()


this let the code run accepting the errors due to the mocking, but would still fail when checking the function call

ameraner · 2024-08-01T11:09:13Z

satpy/tests/reader_tests/test_fci_l1c_nc.py

+                                                          reader_configs, compare_tuples)
+
+    @pytest.mark.parametrize(("channel", "resolution", "compare_tuples"),
+                             [("vis_06", "3km", (1, 10,


changing from 0 to 1 here checks that the chunk numbering is modified correctly

codecov · 2024-08-01T11:13:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.99%. Comparing base (a5c5022) to head (6860030).
Report is 572 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2866      +/-   ##
==========================================
+ Coverage   95.97%   95.99%   +0.01%     
==========================================
  Files         368      368              
  Lines       53973    53985      +12     
==========================================
+ Hits        51801    51821      +20     
+ Misses       2172     2164       -8

Flag	Coverage Δ
behaviourtests	`4.02% <0.00%> (-0.01%)`	⬇️
unittests	`96.08% <100.00%> (+0.01%)`	⬆️

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-08-01T11:30:15Z

Pull Request Test Coverage Report for Build 10302081017

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

86 of 86 (100.0%) changed or added relevant lines in 2 files are covered.
39 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 96.088%

Files with Coverage Reduction	New Missed Lines	%
satpy/scene.py	39	92.4%

Totals
Change from base Build 10140739920:	0.02%
Covered Lines:	52050
Relevant Lines:	54169

💛 - Coveralls

djhoese · 2024-08-06T19:15:42Z

satpy/tests/reader_tests/test_fci_l1c_nc.py

+            except KeyError:
+                # If get_segment_position_info is called, the code will fail with a KeyError because of the mocking.
+                # So we catch the error here for now, but the test will still fail with the following assert_not_called
+                pass


Is there something we can do besides except->pass? Maybe https://docs.python.org/3/library/contextlib.html#contextlib.suppress? Or what about no except at all? I guess maybe I'm confused why a KeyError would get to this level of execution and we shouldn't care about it?

I switched to suppress, that's a good idea, thanks! I also tried to clarify a bit more my comment in the code for why the test is setup like this.

In other words: without the fix of this PR, get_segment_position_info would be called (which we don't want), and would then currently fail with KeyError. My point is that we want to test that the function is not called at all, which is what the test is doing. If instead we would check for the KeyError to happen, someone could technically change get_segment_position_info to avoid the KeyError, the test would then pass, but the bug/unexpected behaviour would likely still be there.

Hhhmmm ok. My other suggestion was going to be a try/finally or to do the assert in the except clause, but then as you said it could be skipped depending on how someone changed the code. Ok. I guess this is good with me then. I do assume you've done way more testing than I will be able to analyze from the code.

…iles can be read at the same time and the highest resolution is picked, as with with the normal data

mraspaud

LGTM, thanks for taking the time to clean up the code also. I really like when we get rid of mocking :)

ameraner added 13 commits August 1, 2024 09:31

remove mocking to enable the actual testing

c6b5c4a

add try-except to pass by the failures related to the mocking

0f77d57

change the count_in_repeat_cycle numbering for the African products t…

ca0d996

…o enable the nominal padding mechanism (which should not be actually activated anyway)

update the test to check that the chunk numbering for the african pro…

18f8c43

…ducts is modified correctly

reformat the test file

70a3838

switch to staticmethod

e18a9bf

clarify resolutions AF variable names

b69f425

remove redundant parenthesis

ebb792a

fix dtype passing

274f961

fix typos

f5f9b42

reformat the reader file

82f44f3

update docstrings

b286074

update more docstrings

5dcc413

ameraner added bug component:readers labels Aug 1, 2024

ameraner self-assigned this Aug 1, 2024

ameraner requested review from gerritholl, djhoese and mraspaud as code owners August 1, 2024 11:03

ameraner commented Aug 1, 2024

View reviewed changes

djhoese reviewed Aug 6, 2024

View reviewed changes

ameraner added 3 commits August 7, 2024 18:24

switch to suppress instead of try/except

a551b35

fix WV filenaming that uses IR instead

04266c3

split AF vis_06 into two filetype for each resolution, so that both f…

6860030

…iles can be read at the same time and the highest resolution is picked, as with with the normal data

mraspaud approved these changes Aug 15, 2024

View reviewed changes

mraspaud merged commit 770cc69 into pytroll:main Aug 15, 2024
17 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FCI L1c reader for African products #2866

Fix FCI L1c reader for African products #2866

ameraner commented Aug 1, 2024 •

edited

Loading

ameraner Aug 1, 2024

ameraner Aug 1, 2024

ameraner Aug 1, 2024

ameraner Aug 1, 2024

codecov bot commented Aug 1, 2024 •

edited

Loading

coveralls commented Aug 1, 2024 •

edited

Loading

djhoese Aug 6, 2024

ameraner Aug 7, 2024 •

edited

Loading

djhoese Aug 7, 2024

mraspaud left a comment

Fix FCI L1c reader for African products #2866

Fix FCI L1c reader for African products #2866

Conversation

ameraner commented Aug 1, 2024 • edited Loading

ameraner Aug 1, 2024

Choose a reason for hiding this comment

ameraner Aug 1, 2024

Choose a reason for hiding this comment

ameraner Aug 1, 2024

Choose a reason for hiding this comment

ameraner Aug 1, 2024

Choose a reason for hiding this comment

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

coveralls commented Aug 1, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10302081017

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

djhoese Aug 6, 2024

Choose a reason for hiding this comment

ameraner Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

djhoese Aug 7, 2024

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

ameraner commented Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

coveralls commented Aug 1, 2024 •

edited

Loading

ameraner Aug 7, 2024 •

edited

Loading