Fix inconsistent behavior of time attributes in EUM L1 GEO readers

[YouvaEUMex]

This is PR is to provide change needed by #2409

• seviri_l1b_native:

  • Apply rounding to current nominal_start_time and nominal_end_time since the information read from the header accounting for the different scan/service modes (temporal resolutions) in order to apply the rounding correctly.

• seviri_l1b_hrit:

  • Move nominal_start_time and nominal_end_time to the time_parameters attribute (currently missing)
  • Add deprecation warnings for the "direct access" to nominal_start_time and nominal_end_time.
  • Apply same type of rounding to the nominal_start_time and nominal_end_time as for the native reader.
  • Add properties observation_start_time and observation_end_time
  • Add to the time_parameters attribute (use the times currently used for start_time and end_time).
  • Make start_time and end_time point to nominal_start_time and nominal_end_time, respectively.
  • Use self.start_time to compute satpos (https://github.com/pytroll/satpy/blob/main/satpy/readers/seviri_l1b_hrit.py#L341)

• fci_l1c_nc:

  • Add properties observation_start_time and observation_end_time (same as current start_time and end_time)
  • Add to the time_parameters attribute (currently missing).
  • Add properties nominal_start_time and nominal_end_time
  • Add to the time_parameters attribute. Here we can get the times in the "human friendly" format by using the repeat cycle number together with information about the different scan/service modes.
    • We should also make sure that the nominal_start_time and nominal_end_time stay the same independent of the number of chunks read(?). --> Using the RC number allow this
  • Make start_time and end_time point to nominal_start_time and nominal_end_time, respectively.

• Closes Inconsistent behavior of time attributes in EUM L1 GEO readers #2409
• Add your name to AUTHORS.md if not there already

[strandgren]

Nice Job! I left a few more comments inline.

Please also rename the PR to something that is more describing and release-note-friendly (usually the PR names are directly added to the list of changes in the release notes of a new version). Also add your name to https://github.com/pytroll/satpy/blob/main/AUTHORS.md

[sjoro]

spotted a few typos, otherwise looks good to me (taken that the tests pass). thanks @YouvaEUMex for your efforts here!

[sfinkens]

Something seems to be wrong in the docstring of seviri_base.round_nom_time:

/home/docs/checkouts/readthedocs.org/user_builds/satpy/conda/2420/lib/python3.10/site-packages/satpy/readers/seviri_base.py:docstring of satpy.readers.seviri_base.round_nom_time:6: ERROR: Unexpected indentation.

[YouvaEUMex]

I do not find a way to reproduce this error can you give some context on when/how it appear ?

[YouvaEUMex]

I do not find a way to reproduce this error can you give some context on when/how it appear ?

issue found and fixed in f21497f

[codecov]

Codecov Report

Merging #2420 (eb8b158) into main (4f6a7c3) will increase coverage by 0.01%.
The diff coverage is 98.63%.

@@            Coverage Diff             @@
##             main    #2420      +/-   ##
==========================================
+ Coverage   94.83%   94.84%   +0.01%     
==========================================
  Files         337      337              
  Lines       49430    49571     +141     
==========================================
+ Hits        46875    47016     +141     
  Misses       2555     2555              

Flag	Coverage Δ
behaviourtests	4.41% <0.00%> (-0.02%)	⬇️
unittests	95.46% <98.63%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
satpy/readers/fci_l1c_nc.py	97.62% <91.30%> (-0.65%)	⬇️
satpy/readers/hrit_base.py	86.60% <100.00%> (+0.36%)	⬆️
satpy/readers/seviri_base.py	100.00% <100.00%> (ø)
satpy/readers/seviri_l1b_hrit.py	91.24% <100.00%> (+0.75%)	⬆️
satpy/readers/seviri_l1b_native.py	86.75% <100.00%> (+0.24%)	⬆️
satpy/readers/seviri_l1b_nc.py	74.35% <100.00%> (+3.83%)	⬆️
satpy/tests/reader_tests/test_hrit_base.py	96.93% <100.00%> (+0.09%)	⬆️
satpy/tests/reader_tests/test_seviri_base.py	100.00% <100.00%> (ø)
satpy/tests/reader_tests/test_seviri_l1b_hrit.py	100.00% <100.00%> (ø)
...y/tests/reader_tests/test_seviri_l1b_hrit_setup.py	100.00% <100.00%> (ø)
... and 3 more

... and 2 files with indirect coverage changes

[sfinkens]

Almost there 🧗 🙂 Can you please update the tests to cover the few missed lines (see coverage comments in the "Files changes" tab)

[sfinkens]

LGTM! Thanks for all the hard work!

[gerritholl]

NB: in #2588 there is discussion as to whether start_time should be observation_start_time or nominal_start_time. Was there a strong reason to choose nominal_start_time and would it be bad if it became observation_start_time instead?

[djhoese]

@gerritholl I think there is a misunderstanding. That is not the problem in #2588.

[strandgren]

@gerritholl The reasoning for using the "nominal" times this is based on the information here https://satpy.readthedocs.io/en/latest/readers.html#time-metadata. The main benefits are that we have human-friendly times harmonized across different sensors that tells from which repeat cycle the data come and that we ensure that other Satpy components get a consistent time for calculations (ex. generation of solar zenith angles).

We had some discussions (can't remember where) about the scenario where only a set of FCI chunks are loaded, but as a start we decided to stick to the nominal range covering the full repeat cycle. I'm not against modifying the behavior when loading a selected number of chunks, but I would be in favour of keeping the current behavior when loading all chunks. Still I guess there is a risk of mis-understanding if the behavior could be different depending on how many chunks you load.