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

combine_metadata only supports the average of time attrs #1174

Closed
zxdawn opened this issue May 2, 2020 · 3 comments · Fixed by #2737
Closed

combine_metadata only supports the average of time attrs #1174

zxdawn opened this issue May 2, 2020 · 3 comments · Fixed by #2737

Comments

@zxdawn
Copy link
Member

zxdawn commented May 2, 2020

Describe the bug
Currently, combine_metadata will calculate the mean value of attrs related to time.
But, start_time should be the minimum and end_time should be the maximum.

Expected behavior
MultiScene:

start_time 2019-07-25 05:15:10
end_time 2019-07-25 05:34:49

Actual results
Scene_1:

start_time 2019-07-25 05:15:10
end_time 2019-07-25 05:24:49

Scene_2:

start_time 2019-07-25 05:25:10
end_time 2019-07-25 05:34:49

MultiScene:

start_time 2019-07-25 05:20:10
end_time 2019-07-25 05:29:49
@djhoese
Copy link
Member

djhoese commented May 2, 2020

I really thought we were already doing that, but you're right of course. Here's the PR where I added averaging #473. A fix for this could probably remove the python 2 compatibility stuff too.

@zxdawn
Copy link
Member Author

zxdawn commented May 2, 2020

@djhoese So, by setting average_times=False will lead to the minimum as start_time and maximum as end_time? If so, why not supporting average_times in timeseries?

res.attrs = combine_metadata(*[x.attrs for x in expanded_ds])

@djhoese
Copy link
Member

djhoese commented May 2, 2020

I'm not sure what you are saying. The combine_metadata function needs to be updated to support min/max'ing start/end_time. I'm not sure what the right way is to handle it in the timeseries function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants