Fix nominal end time in AHI HSD

sfinkens commented 4 months ago

I noticed that for AHI HSD, the attributes start_time/end_time and nominal_start_time/nominal_end_time are identical.

import satpy
import glob

filenames = glob.glob("*.DAT")
scene = satpy.Scene(filenames, reader="ahi_hsd")
scene.load(["B13"])

print(scene["B13"].attrs["start_time"])
print(scene["B13"].attrs["end_time"])
print(scene["B13"].attrs["time_parameters"]["nominal_start_time"])
print(scene["B13"].attrs["time_parameters"]["nominal_end_time"])

Output:

2020-01-01 12:00:00
2020-01-01 12:00:00
2020-01-01 12:00:00
2020-01-01 12:00:00

This has two reasons:

The end_time property returns nominal_start_time. See https://github.com/pytroll/satpy/blob/main/satpy/readers/ahi_hsd.py#L417
The observation_end_time (e.g. 03:10:12.34567 for a full disk scan) is rounded to the "observation timeline" (03:00:00 or "0300" in the header). See https://github.com/pytroll/satpy/blob/main/satpy/readers/ahi_hsd.py#L473. Here, the offset dt only accounts for the start of the scan, not the total scan duration.

To fix this, I modified the nominal_end_time property to add the scan duration to the nominal start time.

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

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (4e85a8f) 95.88% compared to head (1eadfb5) 95.89%. Report is 24 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2742 +/- ## ======================================= Coverage 95.88% 95.89% ======================================= Files 371 371 Lines 52835 52847 +12 ======================================= + Hits 50663 50677 +14 + Misses 2172 2170 -2 ``` | [Flag](https://app.codecov.io/gh/pytroll/satpy/pull/2742/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/2742/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `4.15% <0.00%> (-0.01%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/pytroll/satpy/pull/2742/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `95.99% <100.00%> (+<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 4 months ago

Pull Request Test Coverage Report for Build 7928501394

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

0 of 73 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall first build on fix-time-attrs at 95.973%

Totals
Change from base Build 7801990864:	96.0%
Covered Lines:	50549
Relevant Lines:	52670

💛 - Coveralls

djhoese commented 4 months ago

The observation_end_time (e.g. 03:10:12.34567 for a full disk scan) is rounded to the "observation timeline" (03:00:00 or "0300" in the header). See main/satpy/readers/ahi_hsd.py#L473. Here, the offset dt only accounts for the start of the scan, not the total scan duration.

I'm having trouble understanding this sentence, sorry. When you say "observation_end_time" are you referring to self.observation_end_time or the self.basic_info["observation_end_time"] that comes from the file?

djhoese commented 4 months ago

The new code I think I do understand and looks good to me. One fear is that if the observation times (start or end) coming from the file are ever before the theoretical nominal time then the rounding will give the wrong answer. This isn't something new introduced in this PR, but correct me if I'm wrong:

FLDK theoretical start at 12:00:00. Time in file is 11:59:59.999. Replacing the start time hours/minutes/seconds would put this at 11:50:00, wouldn't it? Or does the timeline variable fix this? Or do we assume that this is impossible? Meaning, the instrument isn't triggered/scheduled to make an observation until 12:00:00 so it could never have a time before this?

sfinkens commented 4 months ago

I'm having trouble understanding this sentence, sorry. When you say "observation_end_time" are you referring to self.observation_end_time or the self.basic_info["observation_end_time"] that comes from the file?

I meant self.observation_end_time, sorry for not being clear. I'll try with more words :) To compute the nominal end time, the reader currently takes self.observation_end_time and replaces hours/minutes/seconds with the "timeline". Then adds an offset dt to get from the "timeline" to the actual start of the scan.

FLDK theoretical start at 12:00:00. Time in file is 11:59:59.999. Replacing the start time hours/minutes/seconds would put this at 11:50:00, wouldn't it? Or does the timeline variable fix this? Or do we assume that this is impossible? Meaning, the instrument isn't triggered/scheduled to make an observation until 12:00:00 so it could never have a time before this?

Oh, nice catch! I think the 11:59 case would still work because hours/minutes/seconds are snapped to the timeline variable. But at midnight it doesn't work anymore, for example

timeline = 0000
observation end time = 2023-01-01 23:59
=> nominal time = 2023-01-01 00:00 (instead of 2023-01-02)

I'll try to take that into account

sfinkens commented 4 months ago

Or do we assume that this is impossible? Meaning, the instrument isn't triggered/scheduled to make an observation until 12:00:00 so it could never have a time before this?

I wouldn't rely on that

djhoese commented 4 months ago

Awesome job. It is surprising and almost sad how much code had to go into get a start and end time, but it is what it is. Thanks for putting all this work into it.

sfinkens commented 4 months ago

Awesome job. It is surprising and almost sad how much code had to go into get a start and end time, but it is what it is. Thanks for putting all this work into it.

Haha, True. But I enjoyed it somehow :smiley:

pytroll / satpy