InfrastructureSystems.jl
InfrastructureSystems.jl copied to clipboard
Issues Deferred from Time Series Redesign
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
resolutionto 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_consistencyand 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 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
@GabrielKS why does using Dates.DateTime(0) as a null value problematic?