Add FCI LSA SAF FRP csv and nc reader support by alexhalbig · Pull Request #3434 · pytroll/satpy

alexhalbig · 2026-07-31T15:22:14Z

Summary

This PR adds tests for the FCI LSA SAF FRP L2 CSV and NC reader.
Including yaml-, utest-, composite-, and enhancement-files

Work finished, based on PWC Spring 2026 Issue - Closes #3350

Testing

pre-commit run --all-files
ruff check . --fix
ruff format .
relevant local unit tests passed

… enhancement files.

codecov · 2026-08-03T08:28:57Z

Codecov Report

❌ Patch coverage is 99.42029% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.36%. Comparing base (17bf08d) to head (e5a5394).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
satpy/readers/fci_lsasaf_frp_l2_nc.py	97.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3434      +/-   ##
==========================================
+ Coverage   96.34%   96.36%   +0.02%     
==========================================
  Files         465      469       +4     
  Lines       59151    59598     +447     
==========================================
+ Hits        56990    57434     +444     
- Misses       2161     2164       +3

Flag	Coverage Δ
behaviourtests	`3.56% <0.00%> (-0.03%)`	⬇️
unittests	`96.45% <99.42%> (+0.02%)`	⬆️

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

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ameraner

Nice job, thank you for this! Just some minor comments/questions/edits, and a requried change for the pixel coordinates.

ameraner · 2026-08-12T08:44:14Z

+            data = data.assign_coords({
+                "longitude": ("y", lons),
+                "latitude": ("y", lats),
+            })


maybe I'm missing something, but what is the point of adding the lon-lat as coordinates on y here, if we then immediately after remap the data on the 2-d array?

You are right. Dimensions are not needed here.
Removed assign_cords()

could it be that the change was not pushed? The assign_coords is still there

… these changes.

alexhalbig · 2026-08-14T07:02:01Z

The CI run still fails in the two TestParallaxCorrectionModifier.test_modifier_interface_cloud_moves_to_observer cases for foroyar. The failure is caused by an unevaluated Dask boolean array:
assert dask.array<all-aggregate, shape=(), dtype=bool, ...>

This behavior surprises me, as the CI tests passed 2 days ago. The changes made, suggested by Andreas Review, should not impact the testing success in my opinion.

My branch is not behind origin/main (HEAD...origin/main = 8 0). Could you please confirm whether this is a known Dask/Pyresample compatibility issue and whether these two tests should be adjusted or marked as expected failures?

ameraner

Thank you for the updates! I noticed a couple of things, see below.

The failing tests are not due to your changes, they're probably due to some library update that broke the parallax tests.. they will need to be fixed separately (feel free to give it a go in another PR if you have time).

ameraner · 2026-08-14T07:51:12Z

+  nc_fci_frp:
+    file_reader: !!python/name:satpy.readers.fci_lsasaf_frp_l2_nc.FRPFileHandler
+    file_patterns:
+     - 'W_PT-{facility_or_tool},SATELLITE,LSA-509_{platform_name}_MTFRPPIXEL_MTG-{coverage}_{disposition_mode}_LPMG_{start_time:%Y%m%d%H%M%S}.nc'


until we really support nc, this should be removed

I thought one step in advance, as I just need to finish the utests for the nc-reader. But I agree with you. Will be removed here, and added later.

If NC support is finished then this thread can be resolved right? My next question though if that's true, should the NC files be listed first in the file_type:s below? I assume the netcdf files are better quality and better performance to read.

Yes agree, they contain more reliant metadata information than parsing information from the filename. I admit, I didn't know the order of the file_types is important.

ameraner · 2026-08-14T07:53:17Z

+            data = data.assign_coords({
+                "longitude": ("y", lons),
+                "latitude": ("y", lats),
+            })


could it be that the change was not pushed? The assign_coords is still there

ameraner · 2026-08-14T08:04:12Z

+        grid = np.full(FRP_GRID_SHAPE, np.nan, dtype=np.float32)
+        grid[rows_int, cols_int] = values
+        xarr = xr.DataArray(
+            grid,
+            dims=("y", "x"),
+            attrs=attrs,
+        )


The grid array here should be daskified to adhere to the normal format of reader outputs, and to optimise memory usage. You can take inspiration from an old version of the LI L2 reader to do that: https://github.com/pytroll/satpy/blame/618fc0e498c7f1e1bfce011f1ed1b041895ed8ce/satpy/readers/li_l2_nc.py#L141-L148
(in that reader, later on we needed an np.add.at functionality - so if you look at the current reader it is much more complex. However you don't need that here, since we have only one value per pixel, hence my link to an older blame version).

Thanks this was helpful. The new version is daskified now.

gerritholl · 2026-08-14T08:06:36Z

I confirm that tests are failing in satpy main after mamba update --all in my local environment. This is unrelated to the current PR, but must be due to a recent dependency update.

gerritholl · 2026-08-14T08:59:13Z

The test failures are unrelated to this PR. Satpy main is also failing. See #3441 .

Merge remote-tracking branch 'upstream/main' into feature-add-frp-mtg-reader

alexhalbig · 2026-09-02T15:17:22Z

@ameraner @mraspaud @djhoese From my perspective, this PR is ready to merge. I have completed the CSV and NC reader support. Could you please take a look?

alexhalbig · 2026-09-02T15:22:02Z

+        values = data_array.data.compute() if hasattr(data_array.data, "compute") else data_array.values
+
+        # Create an empty 1-D nan array for the results
+        flattened_result = np.nan * da.zeros((FRP_GRID_SHAPE[0] * FRP_GRID_SHAPE[1]), dtype=data_array.dtype)


I’ve been thinking about whether this should be implemented with or without np.nan. What do you think?

with np.nan: it becomes a masked array, which can still optionally be filled using fill_value=0

without np.nan: it remains a regular raster, automatically filled with fill_value=0
As it's a fire detection variable, I prefer the first one.

I think a NaN filled raster makes the most sense if I'm understanding you correctly.

djhoese · 2026-09-03T18:27:15Z

+#
+# satpy is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# satpy is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with satpy.  If not, see <http://www.gnu.org/licenses/>.
+


Suggested change

#

# satpy is free software: you can redistribute it and/or modify

# it under the terms of the GNU General Public License as published by

# the Free Software Foundation, either version 3 of the License, or

# (at your option) any later version.

#

# satpy is distributed in the hope that it will be useful,

# but WITHOUT ANY WARRANTY; without even the implied warranty of

# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

# GNU General Public License for more details.

#

# You should have received a copy of the GNU General Public License

# along with satpy. If not, see <http://www.gnu.org/licenses/>.

We just recently relicensed to Apache v2 which does not require a license header on each file. Correct @mraspaud?

djhoese · 2026-09-03T18:27:53Z

@@ -0,0 +1,153 @@
+# Copyright (c) 2022 Satpy developers


Suggested change

# Copyright (c) 2022 Satpy developers

# Copyright (c) 2026 Satpy developers

I'm not sure where we landed on copyright comments with the license change, but either way the year needs to be correct.

djhoese · 2026-09-03T18:35:52Z

+        """Get area definition for fci native 1 km grid."""
+        return get_area_def("mtg_fci_fdss_1km")
+
+    def get_array_on_fci_grid(self, data_array):


I'm not familiar with this data. Could you tell me what abs_line and abs_samp contain? Are they a single value or a 1D array the same size as the variable being gridded?

Looking at the fancy work here I'm wondering/hoping if there is a way to put this into a map_blocks call or something. So the function that dask would call per chunk would generate the NaN filled chunk numpy array, fill in the data related to that chunk, and then return it. If you're already computing the 1D array (or pass it to map_blocks as a single chunk) then it should "just work". If necessary there is also dask's blockwise. My point being that I'm wondering if this could be a bit more lazy.

abs_line represents the index of the active fire pixel, related to the same row in the native FCI projection grid.
Accordingly, abs_samp is the column number (zero based) value in the FCI projection.

map_blocks and/or blockwise sounds worth a test. Maybe even dask.delayed. Anyway, the whole overall processing is already very fast, taking only about 10 seconds. I would consider this as a future improvement step :-)

Sounds good. Semi-recently Dask has actually steered away from mixing Delayed objects and Dask arrays for performance reasons (it ends up duplicating some of the dask graph in some cases). Let's save it for later. that's fine by me.

djhoese · 2026-09-03T18:36:23Z

If these aren't two separate readers then I think it would make sense and follow common practice to put them in the same python module. Same for the tests.

I agree that the tests and readers could be merged into one unified reader. That was my initial intention. However, I quickly noticed this would make the structure more complex to read and maintain.
The CSV files contain fewer variables than the NetCDF files and use a different filename structure. NetCDF provides detailed metadata; CSV provides none, except "encoded" in the filename.
I therefore prefer a simple KISS-style solution with minimal merging and no extensive if/else logic at numerous places. I therefore focused on making the user experience as convenient as possible. Users only need to specify "reader='lsasaf_mtgfrp_l2' " Satpy then determines from the filenames whether the NetCDF or CSV reader should be used.

Unless I'm missing something there doesn't need to be any if/else logic. I'm not talking about merging them into one class. The two classes go in one module. One class gets called FRPCSVFileHandler and the other FRPNetCDFFileHandler and the YAML references those. The variable map global variables get a CSV or NC specific prefix to them. The chunk size and FRP shape variables don't even need to change. The CSV file handler is less than 100 lines of code and the NC reader is ~150. It is very common to have multiple file handlers in a single reader module and it is expected to be that way.

Gotcha, thanks for clarifying. That sounds reasonable. I will change and let you know.

djhoese

I thought I had only a few questions so I made individual comments and then I just kept having more and more to say. Nice work on this. I just have a few questions/suggestions so far.

ameraner

Thank you Alex for adding the NetCDF reader too!
For better flexibility, I would propose to add the with_area_definition mechanism also to this reader, see the equivalent LI L2 implementation

satpy/satpy/readers/li_l2_nc.py

Lines 47 to 51 in 73ae7e4

    
           If needed, the accumulated products can also be accessed as 1-d array by 
        
           setting the reader kwarg ``with_area_definition=False``, 
        
           e.g.:: 
        
             scn = Scene(filenames=filenames, reader="li_l2_nc", reader_kwargs={'with_area_definition': False})

. I would set it as default True, same as for LIL2

satpy/satpy/readers/li_l2_nc.py

Line 81 in 73ae7e4

    
           def __init__(self, filename, filename_info, filetype_info, with_area_definition=True):

.
That way, by default (the current behaviour) you get a 2-d array that can be used as a mask etc. . But if requested through the reader_kwargs, a user can get just the 1-d lists of lat, lon and FRP values. If you still have time to work on this, otherwise it can be added later!

Add MTG FRP csv reader, including reader, yaml, utest, composite, and…

cd7408d

… enhancement files.

alexhalbig requested review from djhoese and mraspaud as code owners July 31, 2026 15:22

Fix reader yaml filename mismatch.

7b36904

alexhalbig added 2 commits August 3, 2026 11:42

Fix RTD formatting problem

f37aead

Remove example code

274a659

ameraner requested changes Aug 12, 2026

View reviewed changes

ameraner assigned alexhalbig Aug 12, 2026

ameraner added enhancement code enhancements, features, improvements component:readers PCW Pytroll Contributors' Week labels Aug 12, 2026

alexhalbig added 5 commits August 13, 2026 15:56

Implement the changes as suggested in the code review. Adapt utest to…

5e94273

… these changes.

Replace former csv-only yaml by csv and nc compatible version

ab42dde

Comment nc part for the moment

3c972bf

Fixed naming mismatch in yaml reader file

b9ac0eb

test CI

0ebcb6c

ameraner requested changes Aug 14, 2026

View reviewed changes

gerritholl mentioned this pull request Aug 14, 2026

Tests failing after upgrading proj 9.7.1 → 9.8.1 #3441

Closed

alexhalbig added 6 commits August 14, 2026 11:53

Daskify 2D output grid. Reduce yaml reader to csv only.

791be74

Trigger CI

a7d42ec

Resolve RtD issue

80a50cd

Merge remote-tracking branch 'upstream/main' into feature-add-frp-mtg-reader

Add FRP NetCDF support to reader. Include fire_confidence variable.

aa68059

Merge branch 'main' into feature-add-frp-mtg-reader

8211e20

enable fire_confidence for nc only

e5a5394

alexhalbig changed the title ~~Add FCI LSA SAF FRP csv reader support~~ Add FCI LSA SAF FRP csv and nc reader support Sep 2, 2026

alexhalbig commented Sep 2, 2026

View reviewed changes

djhoese reviewed Sep 3, 2026

View reviewed changes

djhoese requested changes Sep 3, 2026

View reviewed changes

ameraner reviewed Sep 9, 2026

View reviewed changes

	# Copyright (c) 2022 Satpy developers
	# Copyright (c) 2026 Satpy developers

	If needed, the accumulated products can also be accessed as 1-d array by
	setting the reader kwarg ``with_area_definition=False``,
	e.g.::

	scn = Scene(filenames=filenames, reader="li_l2_nc", reader_kwargs={'with_area_definition': False})

Uh oh!

Conversation

alexhalbig commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

codecov Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ameraner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexhalbig commented Aug 14, 2026

Uh oh!

ameraner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerritholl commented Aug 14, 2026

Uh oh!

gerritholl commented Aug 14, 2026

Uh oh!

alexhalbig commented Sep 2, 2026

Uh oh!

alexhalbig Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexhalbig commented Jul 31, 2026 •

edited

Loading

codecov Bot commented Aug 3, 2026 •

edited

Loading

alexhalbig Sep 2, 2026 •

edited

Loading