-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Define core types #1
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1 +/- ##
=======================================
Coverage ? 58.53%
=======================================
Files ? 1
Lines ? 41
Branches ? 0
=======================================
Hits ? 24
Misses ? 17
Partials ? 0 Continue to review full report at Codecov.
|
) | ||
PTDF = KeyedArray(rand(3, 3); row=bus_names, col=bus_names) | ||
|
||
da_system = SystemDA( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the System
constructors requires a lot of setup (defining instances of all the static components and Dict
s of gens_per_bus
etc). This could make the tests hard to add to in the future. Might there be a better way to structure the test sets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a look through the src/
code, i've lots of "why this structure?" questions
I wouldn't be surprised to learn they're all the right choices, but i'd like to understand the use-cases better. Perhaps linking to existing code patterns downstream that this structure is designed to support would be helpful for that. Ideally some of the answers could then become documentation. Happy to have a call if easiest.
src/system.jl
Outdated
are not time series data). | ||
""" | ||
struct Generators <: StaticComponent | ||
name::Vector{Int} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is name
an Int
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name
is what we call a unit_code
currently and they are Int
s. Given the number of times we iterate through them while constructing the JuMP model it seemed worth keeping them as Int
rather than paying the cost of String
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to change the field name to unit_code
, code
, or id
to make it less confusing? I think most people expect a string type when they see name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, changed to unit_code
src/system.jl
Outdated
startup_cost::Vector{Float64} | ||
shutdown_cost::Vector{Float64} | ||
no_load_cost::Vector{Float64} | ||
time_at_status::Vector{Float64} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we be more specific than time
? is it hours or minutes or seconds or...?
More generally, we might want to document each field, at least with the units / possible values they can take.
DocStringExtensions.jl might be a useful way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the time being it will be in hours, I will add docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time_at_status
should probably be a time-series
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time_at_status
should probably be a time-series
Can you explain why would that be? time_at_status
indicates for how long the unit is in its current state (on/off) at the first time period. We don't know the time at status for all time periods since that will be defined as part of the optimisation (we know this if we have the commitment statuses, e.g. if we're running economic dispatch, but then time_at_status
isn't used at all and it's also trivial to compute)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it is not static; it changes each day. it will have 1 daily value (for the first hour of the day), rather than 24 hourly values, but it's still a changing value, unlike everything else in this struct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it might be because I'm a bit out of the loop – does it matter if something changes across days? Isn't one of the basic assumptions that a system is defined for a given day? Or are we aiming to be as general as possible here?
Also, it's true that start-up/shutdown/no-load costs should not change on a daily basis, but they could change sometimes (not under our current framework, but potentially in the future)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it matter if something changes across days? Isn't one of the basic assumptions that a system is defined for a given day?
yeah, i think either option is fine, since we will a System
will model a single day
But having here only things that are in practice unchanged from day-to-day opens up the possibility of reusing the same StaticComponent across Systems (if that's something we might ever want to do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put time_at_status
and initial_generation
in with the time series data. I think it makes sense to treat them as time series although they will both only have one datetime dimension.
The other thing that links them is they represent information we pass from one model to the next with the logic in FNSims. The time series arrays are a convenient format to perform those _initial_status
and _update_system_generation
operations on.
src/system.jl
Outdated
x datetimes. Only generators that provide each service are included in the array. | ||
""" | ||
struct ServicesTimeSeries | ||
reg::KeyedArray{Float64} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are the consequences of using KeyedArray
here rather than e.g. AxisArrays
? Does it have any consquences for building JuMP models like @variable(model, reg[g in gens, t in datetimes] >= 0)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the syntax with a KeyedArray
, for example when defining a constraint, would look like:
D = system.loads
@constraint(
model,
energy_balance[t in datetimes],
sum(p[g, t] for g in unit_codes) == sum(D(f, t) for f in axiskeys(D, 1))
)
Note the ()
syntax where we access D
. Whereas with AxisArrays
this would be D[f, t]
. Both KeyedArray
and AxisArray
interact badly with JuMP
in performance terms. Some notes on that in the design doc. I've found one solution would be to convert D
to a JuMP.Containers.DenseAxisArray
before using it to define constraints.
src/system.jl
Outdated
|
||
Returns three vectors containing of the names of branches which have 0, 1, and 2 breakpoints. | ||
""" | ||
function branches_by_breakpoints(branches::Branches) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the use-case for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constraints concerning branch flows e.g. slacks.
src/system.jl
Outdated
Type for static generator component attributes (i.e. things that describe a generator that | ||
are not time series data). | ||
""" | ||
struct Generators <: StaticComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these types compare to those in PowerFlowData.jl? Does this have any info not contained in PowerFlowData.Generators
?
Is it viable to have a constructor Generators(::PowerFlowData.Generators) = ...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there would be some things missing from PowerFlowData.Generators
because we gather generator data in FNDP from several datasets other than the PSSE (like offer curves and the deanonymised generator table).
src/system.jl
Outdated
abstract type StaticComponent end | ||
|
||
Base.length(components::StaticComponent) = length(getfield(components, 1)) | ||
# define interfaces? Iterator? Table? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me like the data has been split up exactly to facilitate the Tables.jl interface, i.e. we split "static" data (table-like) and time-series data, which suggests to me we think there's a use-case for having the "static" data available to work with as a table... Do we have such a use-case? Where e.g. df = DataFrame(gens::Generators)
would be a useful thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a use case for Branches
, see branches_to_breakpoints
function.
For Generators
we have a lot of accessor functions in FNModels (get_ramp_rates
, get_startup_limits
) that return a Dict
linking the id of the generator to the value of its ramp rate, startup limit etc. That pattern suggested to me that all these "static" attributes of generators could be stored in a column table to be accessed when needed.
src/system.jl
Outdated
end | ||
|
||
""" | ||
gens_per_zone(gens::Generators) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the use-case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regulation requirement constraints are defined by zone so we need to know which generators are in which zones.
Co-authored-by: Nick Robinson <npr251@gmail.com>
src/system.jl
Outdated
min_downtime::Vector{Float64} | ||
ramp_up::Vector{Float64} | ||
ramp_down::Vector{Float64} | ||
initial_gen::Vector{Float64} # this one changes in RT with _update_system_generation - but is that necessary - could be a mutable time series? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial_gen
is not static, so probably should not be in the StaticComponent
, although unlike other time-series which are N x 24 (hours), this is a daily value so will be a N x 1 Matrix / Vector... might need a bit of further thought, but i suspect we want to treat it like the time-series data
src/system.jl
Outdated
rate_a::Vector{Float64} | ||
rate_b::Vector{Float64} | ||
is_monitored::Vector{Bool} | ||
break_points::Vector{Tuple{Vararg{Float64}}} # variable length (0, 1, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth these being fixed length Vector{NTuple{2, Float64}}
but with trailing 0
in the length 0 and length 1 cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to a two tuple. Trailing zeros should work fine as we can use them to distinguish between branches with 0, 1, or 2 breakpoints.
Summary of changes after first round of review:
|
src/system.jl
Outdated
number::Int64 | ||
"Zonal regulation requirement (MWs)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these MW
or MWh
?
Also we should not pluralise units
"Zonal regulation requirement (MWs)" | |
"Zonal regulation requirement (MW)" |
or
"Zonal regulation requirement (MWs)" | |
"Zonal regulation requirement (MWh)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're generally lax with MW vs MWh because we model things hourly, which means that if p = 100
that could either mean that the generator has a power output of 100 MW for one hour, or that the generator produced a total of 100 MWh across that hour (same logic applies virtually everywhere). I think it's generally easier to see it as MW and follow the logic that we're assuming things are static within a single time period (which in our case is one hour)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to use MW
src/system.jl
Outdated
LODF::Dict{String, KeyedArray} | ||
PTDF::KeyedArray{Float32, 2} | ||
"`Dict` where the keys are bus names and the values are generator ids at that bus" | ||
gens_per_bus::Dict{InlineString15, Vector{Int}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found type aliases to be very helpful in cases like this (example),
where we are working with basic/generic types (String
, Int8
, whatever) that are not themselves informative about what they're being used to represent, and which we need to be consistent across many locations
e.g. if we have Bus.name isa String
then we need eltype(keys(incs_per_bus)) == String
, then i think both easier to maintain and clearer to have
const BusName = String
struct Bus
name::BusName
...
end
struct System
incs_per_bus::Dict{BusName, V}
...
end
src/system.jl
Outdated
"`Dict` where the keys are bus names and the values are generator ids at that bus" | ||
gens_per_bus::Dict{InlineString15, Vector{Int}} | ||
"`Dict` where the keys are bus names and the values are load ids at that bus" | ||
loads_per_bus::Dict{InlineString15, Vector{String}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to use Dictionary
here (rather than mixing Dict
and Dictionary
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use Dictionary
, it does seem nicer than mixing the two.
src/system.jl
Outdated
return zero_bp, one_bp, two_bp | ||
struct Branch | ||
"Branch long name" | ||
name::InlineString31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The names get as long as 23 characters. Is it still worth specifying an InlineString
type at that length? I believe the benefit diminishes.
I think so far this is looking nice. julia> using Dictionaries, DataFrames
julia> components = [
(name="Foo", value=1),
(name="Bar", value=2),
]
2-element Vector{NamedTuple{(:name, :value), Tuple{String, Int64}}}:
(name = "Foo", value = 1)
(name = "Bar", value = 2)
julia> dd = index(first, components)
2-element Dictionary{String, NamedTuple{(:name, :value), Tuple{String, Int64}}}
"Foo" │ (name = "Foo", value = 1)
"Bar" │ (name = "Bar", value = 2)
julia> Tables.istable(dd)
true
julia> DataFrame(dd)
2×2 DataFrame
Row │ name value
│ String Int64
─────┼───────────────
1 │ Foo 1
2 │ Bar 2
julia> dd["Foo"]
(name = "Foo", value = 1) |
src/system.jl
Outdated
# Virtuals/PSD time series | ||
"Increment bids time series data. `KeyedArray` where the axis keys are `bid ids x datetimes`" | ||
increment_bids::KeyedArray{Vector{Tuple{Float64, Float64}}, 2} | ||
"Decrement bids time series data. `KeyedArray` where the axis keys are `bid ids x datetimes`" | ||
decrement_bids::KeyedArray{Vector{Tuple{Float64, Float64}}, 2} | ||
"Price sensitive demand time series data. `KeyedArray` where the axis keys are `bid ids x datetimes`" | ||
price_sensitive_demand::KeyedArray{Vector{Tuple{Float64, Float64}}, 2} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being a bit pedantic but PSDs are also bids, so it sounds weird to have increment_bids
, decrement_bids
, and price_sensitive_demand
. Perhaps just remove the _bids
suffix altogether to avoid long names?
Fields: | ||
$TYPEDFIELDS | ||
""" | ||
struct SystemRT <: System |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SystemDA
and SystemRT
are pretty large and almost identical except for a couple fields that are present in one but not the other. Are we fine with this? Isn't there a more elegant solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could wrap these up to structs to make clear the differences
e.g.
struct SystemDA
generators::Dictionary{InlineString15, Generator}
generator_statuses::GeneratorStatusesDA
generator_timeseries::GeneratorTimeSeries
...
end
struct SystemRT
generators::Dictionary{InlineString15, Generator}
generator_statuses::GeneratorStatusesRT
generator_timeseries::GeneratorTimeSeries
...
end
struct GenStatusesDA
"Hours each generator has been at its current status at the start of the day"
hours_at_status::KeyedArray{Float64, 1}
"Generator availability"
availability::KeyedArray{Bool, 2}
"Generator must run flag indicating that the generator has to be committed at that hour"
must_run::KeyedArray{Bool, 2}
end
struct GenStatusesRT
"Generator status indicated by a `Bool`"
status::KeyedArray{Bool, 2}
"Generator ancillary service status indicated by a `Bool`"
status_regulation::KeyedArray{Bool, 2}
end
struct GeneratorTimeSeries
"Generation of the generators at the start of the time period (MWs)"
initial_generation::KeyedArray{Float64, 1}
"Generator offer curves. `KeyedArray` where the axis keys are `generator names x datetimes`"
offer_curve::KeyedArray{Vector{Tuple{Float64, Float64}}, 2}
"Generator minimum output in the ancillary services market (MWs)"
regulation_min::KeyedArray{Float64, 2}
"Generator maximum output in the ancillary services market (MWs)"
regulation_max::KeyedArray{Float64, 2}
"Generator minimum output (MWs)"
pmin::KeyedArray{Float64, 2}
"Generator maximum output (MWs)"
pmax::KeyedArray{Float64, 2}
"Ancillary services regulation offer prices (\$ /MW)"
asm_regulation::KeyedArray{Float64, 2}
"Ancillary services spinning offer prices (\$ /MW)"
asm_spin::KeyedArray{Float64, 2}
"Ancillary services supplemental on offer prices (\$ /MW)"
asm_sup_on::KeyedArray{Float64, 2}
"Ancillary services supplemental off offer prices (\$ /MW)"
asm_sup_off::KeyedArray{Float64, 2}
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, the System
structs are pretty large and highlighting the difference between DA and RT would be good. I would prefer to merge this and introduce new structs in the next PR, as that will involve a fair amount of code shuffling.
I've set the version as -DEV
so nothing will be tagged yet.
Co-authored-by: Raphael Saavedra <raphael.saavedra93@gmail.com>
Define types to be used for modelling market clearing.