search

NREL-Sienna / InfrastructureSystems.jl

Utility package for Sienna's simulation infrastructure
https://nrel-sienna.github.io/InfrastructureSystems.jl/
BSD 3-Clause "New" or "Revised" License
35 stars 20 forks source link

Deterministic Type Hierarchy #378

Open GabrielKS opened 3 weeks ago

GabrielKS commented 3 weeks ago

I don't love how we special-case 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. Broken out of https://github.com/NREL-Sienna/InfrastructureSystems.jl/issues/356.

GabrielKS commented 3 weeks ago

We've resolved to not make any backwards-incompatible changes due to this right now, but for the next major version, here is my proposal:

Currently we have this type hierarchy:

AbstractDeterministic
  |- Deterministic  # this one is a "real" forecast
  |- DeterministicSingleTimeSeries  # this one is a "fake" forecast

and these behaviors:

  1. AbstractDeterministic doesn't ever get referred to by the user
  2. When the user says Deterministic, it means the Deterministic time series of that name, if there is none then the DeterministicSingleTimeSeries of that name, if there is none then error

I propose to make the following renames:

AbstractDeterministic RENAME TO Deterministic
  |- Deterministic RENAME TO DeterministicTrueForecast  # or something else, not wedded to this
  |- DeterministicSingleTimeSeries NO RENAME

and have the following behaviors:

  1. When the user wants to refer to something that behaves like a deterministic forecast regardless of whether the underlying data is really forecast data or not, they say Deterministic, which is now an abstract type. Queries on Deterministic return the DeterministicTrueForecast or DeterministicSingleTimeSeries with that name if there is only one, error if there are both, in which case the user has to specify a subtype to clear up the ambiguity.
    • In fact, this works for all abstract types, e.g., queries on Forecast return the Forecast with that name no matter its concrete type if there is only one, error if there are multiple
  2. If we want to minimize required user code changes at the expense of abuse of notation, create an alias of the DeterministicTrueForecast constructor called Deterministic. I would prefer my proposal without this, but I think my proposal with this is still better than the status quo

This proposal ends the abuse of the type hierarchy and the hard-coding that requires. The cost in change to the user interface is either practically zero, if the constructor alias is created, or small if it is not. The benefit is in more elegant and maintainable code and an intuitive connection between the type hierarchy and the time series query matching spec.