Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nominal end time in AHI HSD #2742

Merged
merged 12 commits into from
Feb 16, 2024
Merged

Fix nominal end time in AHI HSD #2742

merged 12 commits into from
Feb 16, 2024

Conversation

sfinkens
Copy link
Member

@sfinkens sfinkens commented Feb 12, 2024

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:

  1. The end_time property returns nominal_start_time. See https://github.com/pytroll/satpy/blob/main/satpy/readers/ahi_hsd.py#L417
  2. 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

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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
@@           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 Coverage Δ
behaviourtests 4.15% <0.00%> (-0.01%) ⬇️
unittests 95.99% <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.

@sfinkens sfinkens changed the title Fix time attrs Fix nominal end time in AHI HSD Feb 12, 2024
@sfinkens sfinkens marked this pull request as ready for review February 12, 2024 11:16
@coveralls
Copy link

coveralls commented Feb 12, 2024

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.

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 Coverage Status
Change from base Build 7801990864: 96.0%
Covered Lines: 50549
Relevant Lines: 52670

💛 - Coveralls

@djhoese
Copy link
Member

djhoese commented Feb 12, 2024

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
Copy link
Member

djhoese commented Feb 12, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mraspaud mraspaud added the bug label Feb 15, 2024
@mraspaud mraspaud added this to the v0.47.0 milestone Feb 15, 2024
satpy/readers/ahi_hsd.py Outdated Show resolved Hide resolved
@djhoese
Copy link
Member

djhoese commented Feb 16, 2024

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.

@djhoese djhoese merged commit e74729e into pytroll:main Feb 16, 2024
17 of 19 checks passed
@sfinkens
Copy link
Member Author

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 😃

@sfinkens sfinkens deleted the fix-time-attrs branch October 18, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants