-
Notifications
You must be signed in to change notification settings - Fork 35
Description
Before NREL-Sienna/PowerSystems.jl#1297, PSY.MarketBidCost.shut_down
was of type Float64
. If you called the constructor with an int, the convert
machinery would automatically convert it to a float. That PR makes shut_down
a Union{TimeSeriesKey, Float64}
and that automatic process breaks down:
ERROR: MethodError: Cannot `convert` an object of type
Int64 to an object of type
Union{Float64, TimeSeriesKey}
The function `convert` exists, but no method is defined for this combination of argument types.
because it doesn't automatically know whether it should convert to Float64
or TimeSeriesKey
. See https://discourse.julialang.org/t/union-type-in-struct-type-conversion/42276 and JuliaLang/julia#51697.
Typically when this happens, we add a bunch of auxiliary constructors to manually perform the conversions, and that's what I did in that PR. But this process is cumbersome and error-prone, is not compatible with @kwdef
, and it takes some care to avoid method ambiguities in the case where you might be manually performing multiple conversions for different fields of the same instance.
I claim that for union types where we own at least one of the constituent types and depend on the rest, it is not type piracy to define things like
Base.convert(::Type{Union{Float64, TimeSeriesKey}}, x::Integer) = convert(Float64, x)
Base.convert(::Type{Union{Nothing, Float64, TimeSeriesKey}}, x::Integer) =
Base.convert(::Type{Union{Nothing, Float64}, x}) # RHS method is already defined in Base
: this is just saying "it doesn't convert to one of our types, it must convert to one of yours." I propose that we define such methods for common Sienna union types, in the packages where the constituent types we own are defined, to eliminate the need for so many of these auxiliary constructors.