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

Resolution parameter for TimeSeriesVariableResolution #319

Open
jkiviluo opened this issue Nov 24, 2023 · 11 comments
Open

Resolution parameter for TimeSeriesVariableResolution #319

jkiviluo opened this issue Nov 24, 2023 · 11 comments
Labels
enhancement New feature or request
Milestone

Comments

@jkiviluo
Copy link
Member

jkiviluo commented Nov 24, 2023

At the moment resolution of each timestep in TimeSeriesVariableResolution needs to be calculated from the datetimes. It would be handy to have a resolution parameter (preferrably in timedelta, but that's another issue), from which the user can get the resolution of each timestep directly (and avoid calculating it every time variable time series are used).

This would then force the user to indicate what is the duration of the last time step, since it is not defined when using only datetime series. When creating a TimeSeriesVariableResolution, there would be a mandatory parameter for the last_time_step_duration. The other (than the last time step) resolutions can be calculated from the datetime series when the parameter is created.

@jkiviluo jkiviluo added the enhancement New feature or request label Nov 24, 2023
@jkiviluo jkiviluo added this to the v0.8.2 milestone Nov 24, 2023
@jkiviluo jkiviluo transferred this issue from spine-tools/Spine-Toolbox Nov 24, 2023
@manuelma
Copy link
Collaborator

manuelma commented Nov 24, 2023

It would be handy to provide a duration parameter

Do you mean, to ask the user to provide the duration of the time-step? I thought this was about adding a method to the class so the user could extract all the durations from the provided timestamps.

(We should use the word resolution instead of duration though, because that's the word in the time-series specification.)

@jkiviluo jkiviluo changed the title Duration parameter for TimeSeriesVariableResolution Resolution parameter for TimeSeriesVariableResolution Nov 24, 2023
@jkiviluo
Copy link
Member Author

Do you mean, to ask the user to provide the duration of the time-step?

Yes, the resolution of the last time step (as the other can be calculated). I tried to clarify the issue description.

@manuelma
Copy link
Collaborator

manuelma commented Nov 27, 2023

Thank you @jkiviluo I think that's clear now. So there are two independent things it seems:

  1. Add a resolution read-only property to TimeSeriesVariableResolution that calculates and returns the resolution based on the timestamps ([next_t - t for (t, next_t) in zip(self.indexes[:-1], self.indexes[1:])])
  2. Modify the time-series parameter value spec to include a new mandatory property for time-series where the index is given explicitely. The new property is a duration value that indicates the duration of the last time-series value. Then add support for this new property in TimeSeriesVariableResolution.

@soininen
Copy link
Collaborator

I guess we also need to update the JSON specification and parameter_value_format.rst.

@manuelma
Copy link
Collaborator

You're right @soininen - but I am not sure this solution is the nicest. Typically when I see time-series in data, the data doesn't include something like this (the duration of the last time-step). Maybe we are doing something wrong.

@soininen
Copy link
Collaborator

Maybe we are doing something wrong.

I feel the same. I would rather interpret the last time stamp as the end time than add a 'last step duration' which seems to complicate things.

@manuelma
Copy link
Collaborator

My impression is one should look at time-series as discrete functions, rather than continuous. The data itself doesn't say what happens in between points or beyond the first and last points - this is up to interpretation. So I'd rather add an explicit interpolation/extrapolation method property with a nice default, rather than duration-of-last-time-step.

@soininen
Copy link
Collaborator

As a physicist, I would choose zeroth order extrapolation as a nice default, i.e. the last value would be valid indefinitely.

@jkiviluo
Copy link
Member Author

My reason to prefer explicit duration for the last time step was to make the user aware of what they are choosing. I think users might often not realized that they would need to give a time step for the last duration. Typically this would mean that they give 1.1.2020 00:00:00 or something similar. However, that timestamp should then not contain data. And I feel bit uncomfortable with that.

I don't really know - both approaches have their benefits and drawbacks.

@soininen
Copy link
Collaborator

However, that timestamp should then not contain data.

I forgot this so maybe it is not a solution unless we are ready to redefine our time series as N data points with N+1 time stamps.

@jkiviluo
Copy link
Member Author

redefine our time series as N data points with N+1 time stamps

Possible solution. This would concern only TimeSeriesVariableResolution afaik.

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

No branches or pull requests

3 participants