spine-tools / Spine-Database-API

Database interface to Spine generic data model
https://www.tools-for-energy-system-modelling.org/
GNU Lesser General Public License v3.0
6 stars 5 forks source link

Resolution parameter for TimeSeriesVariableResolution #319

Open jkiviluo opened 7 months ago

jkiviluo commented 7 months ago

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.

manuelma commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

manuelma commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

jkiviluo commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

Possible solution. This would concern only TimeSeriesVariableResolution afaik.