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

Using a static image alters time information #2734

Closed
pnuu opened this issue Jan 31, 2024 · 4 comments · Fixed by #2737
Closed

Using a static image alters time information #2734

pnuu opened this issue Jan 31, 2024 · 4 comments · Fixed by #2737
Assignees
Labels
component:compositors component:scene enhancement code enhancements, features, improvements

Comments

@pnuu
Copy link
Member

pnuu commented Jan 31, 2024

Feature Request

Creating a composite that uses a static background image yields an image with a different time than the used satellite data. The current approach sets the start_time of the static image to utcnow(), and satpy.dataset.combine_metadata takes average of this and the other dataset used in the creation of the composite. Creating images in NRT might not change the time stamp a lot, but doing the same for data 2 days ago back already shifts the result by one full day.

Describe the solution you'd like
Instead of always taking the average of the times, could we instead have multiple options:

  • min(datetime_list) - perfect when using timed data with static background, always gives the time of the actual data
  • mean(datetime_list) - when multiple instruments with differing times are combined
  • max(datetime_list) - when multiple instruments with differing times are combined, and the more recent one has the more representative time

Could we use the Satpy config system for controlling this? I would argue the default should be min(), but I'm open to comments.

Implementation it self is simple: add new min/max functions and call min/mean/max depending on the setting.

Describe any changes to existing user workflow
No compatibility issues if mean is kept as default. Setting the config item would need one additional line of code in scripts when required.

The only place where combine_metadata(..., average_times=False) is used is in a test method.

Additional context
I was thinking this could also be a bug, but in the end decided a feature request would be better instead.

@pnuu pnuu added enhancement code enhancements, features, improvements component:compositors component:scene labels Jan 31, 2024
@pnuu pnuu self-assigned this Jan 31, 2024
@djhoese
Copy link
Member

djhoese commented Jan 31, 2024

Kind of a duplicate of #2630

I think the question is whether or not this is an option for all datetime types in the metadata or just start_time and end_time. There are other times in the metadata now (observation, nominal, etc), but if we're merging datasets and these differ do we really want to min/max/mean these at all? The alternative is to always min the start_time and max the end_time.

@pnuu
Copy link
Member Author

pnuu commented Feb 1, 2024

Damn, missed #2630 .

I'd be happy with automatic min(start_times) and max(end_times). Currently all attributes having "time" in their names are averaged, so as a first guess we could keep those as-is.

I'll create a PR and link these two issues to it.

@djhoese
Copy link
Member

djhoese commented Feb 1, 2024

Or maybe support of a None start time would be better? Less "fake" data.

@pnuu
Copy link
Member Author

pnuu commented Feb 1, 2024

I added also handling for None as time. PR coming in a second.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:compositors component:scene enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants