Share FCI L1c metadata between segments

pnuu commented 3 months ago

Some of the metadata are identical in every FCI L1c segment, so reading those only once is possible. This will save a lot of time in Scene creation when the data are in S3 storage:

main - 37.0 s
PR - 23.0 s

There will be conflicts with https://github.com/pytroll/satpy/pull/2686, but maybe adding the pickle would benefit also this feature.

[ ] Closes #xxxx
[x] Tests added
[ ] Fully documented
[ ] Add your name to AUTHORS.md if not there already

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 96.87500% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.94%. Comparing base (ee2273a) to head (f46df94). Report is 10 commits behind head on main.

Files	Patch %	Lines
satpy/readers/fci_l1c_nc.py	91.30%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2828 +/- ## ======================================= Coverage 95.94% 95.94% ======================================= Files 366 366 Lines 53515 53580 +65 ======================================= + Hits 51343 51409 +66 + Misses 2172 2171 -1 ``` | [Flag](https://app.codecov.io/gh/pytroll/satpy/pull/2828/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | Coverage Δ | | |---|---|---| | [behaviourtests](https://app.codecov.io/gh/pytroll/satpy/pull/2828/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `4.04% <0.00%> (-0.01%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/pytroll/satpy/pull/2828/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `96.04% <96.87%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9515844065

Details

9 of 28 (32.14%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 96.006%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/fci_l1c_nc.py	4	23	17.39%
<!--	Total:	9	28	32.14%	-->

Totals
Change from base Build 9513521871:	-0.03%
Covered Lines:	51566
Relevant Lines:	53711

💛 - Coveralls

pnuu commented 3 months ago

I'll see what I can do for testing. It's not easy, because the file handler is never used in the tests.

pnuu commented 3 months ago

I've now added two tests that hopefully show that the storing and reusing of common items between the FCI L1c segments work.

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9581505593

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

48 of 51 (94.12%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.002%) to 96.038%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/fci_l1c_nc.py	20	23	86.96%
<!--	Total:	48	51	94.12%	-->

Totals
Change from base Build 9513521871:	-0.002%
Covered Lines:	51605
Relevant Lines:	53734

💛 - Coveralls

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9592333785

Details

48 of 51 (94.12%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.002%) to 96.039%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/fci_l1c_nc.py	20	23	86.96%
<!--	Total:	48	51	94.12%	-->

Totals
Change from base Build 9588363290:	-0.002%
Covered Lines:	51621
Relevant Lines:	53750

💛 - Coveralls

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9592773461

Details

49 of 52 (94.23%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.002%) to 96.039%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/fci_l1c_nc.py	20	23	86.96%
<!--	Total:	49	52	94.23%	-->

Totals
Change from base Build 9588363290:	-0.002%
Covered Lines:	51622
Relevant Lines:	53751

💛 - Coveralls

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9593191408

Details

62 of 64 (96.88%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.006%) to 96.047%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/fci_l1c_nc.py	21	23	91.3%
<!--	Total:	62	64	96.88%	-->

Totals
Change from base Build 9588363290:	0.006%
Covered Lines:	51638
Relevant Lines:	53763

💛 - Coveralls

simonrp84 commented 3 months ago

Interesting! In the code I can see some metadata listed as non-shareable. Do you have a list of the shareable metadata? i.e: Which ones don't change between segments and are hence affected by this PR?

pnuu commented 3 months ago

Anything in the list shown here that don't end in the strings listed in NONSHAREABLE_VARIABLE_ENDINGS can be shared between the segments. Note that the {chan_name} is replaced with the 16 channels FCI has, so the list is quite long.

gerritholl commented 3 months ago

Interesting! In the code I can see some metadata listed as non-shareable. Do you have a list of the shareable metadata? i.e: Which ones don't change between segments and are hence affected by this PR?

In https://github.com/pytroll/satpy/pull/2686/files#diff-2170f8edf16088150763d5f3a6cbd69d62600d238c0ba80a41afcb4832fb7b5d I explicitly list what metadata can be shared between segments.

pnuu commented 3 months ago

In addition, could more implementation be in netcdf_utils so it can be used by other readers where relevant?

I think so. As I said in another comment above this snowballed from a quick test at PCW so I touched as little other parts of the code as possible. The FCI L1c data format is the most demanding in this regard, and most important to me, so started here.

I'll see about generalizing things later and converting this as a draft for the summer period.

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9596154108

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

62 of 64 (96.88%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.006%) to 96.047%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/fci_l1c_nc.py	21	23	91.3%
<!--	Total:	62	64	96.88%	-->

Totals
Change from base Build 9588363290:	0.006%
Covered Lines:	51638
Relevant Lines:	53763

pytroll / satpy