[ ] Currently, time series resolution is validated when adding a time series to a manager, and this consists of comparing the stated resolution to the difference between the first two timestamps. This sort of validation should instead be done in the time series constructor, and it should compare the differences between all consecutive timestamps, so it is never possible to construct an invalid time series. More radically, one might ask why we are storing this redundant information at all rather than having a getter that computes it. See https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1578585655 and https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1578613576
[ ] #378: I don't love how we special-case (in this PR and in existing code) that Deterministic, a concrete type, sometimes also refers to DeterministicSingleTimeSeries, a different concrete type. I don't know about the typical user, but this definitely confused me before it was explained to me. We would have to think about how to resolve this without breaking existing code. For now we'll keep it as-is but at some point restore the behavior that you can pass abstract types to get_time_series; when that is done, there is a change that can be reverted in PSY. See https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1581220010
Here are a few issues that came up in https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349 that didn't quite merit being dealt with there but should probably be addressed at some point. Some of these might be spun off into their own issues.
resolution
to the difference between the first two timestamps. This sort of validation should instead be done in the time series constructor, and it should compare the differences between all consecutive timestamps, so it is never possible to construct an invalid time series. More radically, one might ask why we are storing this redundant information at all rather than having a getter that computes it. See https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1578585655 and https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1578613576check_consistency
and allow the user to override. See https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1579842382Dates.DateTime(0)
as a null value, which could be problematic. This existed in the codebase before this PR so we haven't changed it yet. See https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1579859374Deterministic
, a concrete type, sometimes also refers toDeterministicSingleTimeSeries
, a different concrete type. I don't know about the typical user, but this definitely confused me before it was explained to me. We would have to think about how to resolve this without breaking existing code. For now we'll keep it as-is but at some point restore the behavior that you can pass abstract types toget_time_series
; when that is done, there is a change that can be reverted in PSY. See https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1581220010