InfrastructureSystems.jl icon indicating copy to clipboard operation
InfrastructureSystems.jl copied to clipboard

Issues Deferred from Time Series Redesign

Open GabrielKS opened this issue 1 year ago • 2 comments

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.

  • [ ] 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
  • [x] #360
  • [ ] We might want to check/enforce somewhere that features are not of mixed type. Or we might not. Current plan is to add a check for this in check_consistency and allow the user to override. See https://github.com/NREL-Sienna/InfrastructureSystems.jl/pull/349#discussion_r1579842382
  • [ ] We are using Dates.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_r1579859374
  • [ ] #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

GabrielKS avatar Apr 26 '24 16:04 GabrielKS

@GabrielKS why does using Dates.DateTime(0) as a null value problematic?

jd-lara avatar Apr 26 '24 22:04 jd-lara