Avi/initial plus sp1 general #26
Conversation
| Kwonly = "18d08c8c-0732-55ee-a446-91a51d7b4206" | ||
| Plots = "91a5bcdd-55d7-5caf-9e0b-520d859cae80" | ||
| XLSX = "fdbf4ff8-1666-58a4-91e7-1b58723a45e0" | ||
|
|
||
| [compat] | ||
| julia = "1.6" |
There was a problem hiding this comment.
are you sure you wanna go with Julia 1.6? Is that necessary?
There was a problem hiding this comment.
Will update at a later stage. Would rather be conservative on this.
| hot_utilities_dict::Dict{String, SimpleHotUtility} | ||
| cold_utilities_dict::Dict{String, SimpleColdUtility} | ||
| ΔT_min::Float64 | ||
| @add_kwonly function ClassicHENSProblem(hot_streams_dict, cold_streams_dict, hot_utilities_dict = Dict{String, SimpleHotUtility}(), cold_utilities_dict = Dict{String, SimpleColdUtility}(); ΔT_min = 10) |
There was a problem hiding this comment.
do you have a good reason to make this an inner constructor? Usually inner constructors are used if you want to force some sort of sanity check that cannot be sidestepped. doesn't seem to be the case here. In that case it just reduces the extensibility but adds no value otherwise. Not sure if really critical here, though.
There was a problem hiding this comment.
- Difficult choice. I am inclined to keep the inner constructor as I may need to have these sanity checks as this constructor directly parses user information. Also, in the worst case if I find it hinders extensibility, its easier to delete than add it.
|
|
||
| Holds the classical HENS problem with fixed stream data. | ||
| """ | ||
| mutable struct ClassicHENSProblem <: AbstractSynthesisProblem |
There was a problem hiding this comment.
does this need to be mutable? Since it holds the dictionaries already it feels like there is little value? Perhaps one wants to adjust
There was a problem hiding this comment.
Ok, made it immutable
|
|
||
| Single hot stream | ||
| """ | ||
| mutable struct HotStream <: AbstractStream |
There was a problem hiding this comment.
the mutability of this I like. I could see adjusting stream data to be an important use case even after the problem has been defined.
| """ | ||
| mutable struct HotStream <: AbstractStream | ||
| name::String # May eventually need parametric types when these can be variables. | ||
| T_in::Float64 |
There was a problem hiding this comment.
is it necessary to restrict to Float64? What if someone uses Float32 for whatever reason or inputs integer data for temperatures. Maybe make the type parametric, and handle promotion to the same number type for all inputs?
There was a problem hiding this comment.
Good point to use parametric types. #28
I do accept any type in the constructor and promote to Float64 as seen in the constructor code.
| Supports multiple utilities if appropriately matched to interval. | ||
| [TODO: Utility costs] | ||
| """ | ||
| function solve_minimum_utilities_subproblem(prob::ClassicHENSProblem; time_limit = 60.0, presolve = true, optimizer = HiGHS.Optimizer, verbose = true) |
There was a problem hiding this comment.
The use of macros to create the problem is very slow. Avoiding that is a lot of work and takes a lot of learning about JuMP (or ideally MathOptInterface) though. Depending on how big the problems become, the inefficiency of macro usage can actually become very noticeable (especially when you start thinking about the nonlinear problems). For now I think this is fine but something to keep in mind.
There was a problem hiding this comment.
Yes, will make an issue #27
| hot_utilities_contribs::Dict{String, Float64} | ||
| """Cold utility entering system at interval. Note: Each CU can only enter at one interval""" | ||
| cold_utilities_contribs::Dict{String, Float64} | ||
| @add_kwonly function TemperatureInterval(index, T_hot_upper, T_hot_lower, T_cold_upper, T_cold_lower, R_in = 0.0, R_out = 0.0, hot_streams_contribs = Dict{String, Float64}(), cold_streams_contribs = Dict{String, Float64}(), hot_utilities_contribs = Dict{String, Float64}(), cold_utilities_contribs = Dict{String, Float64}()) |
There was a problem hiding this comment.
inner constructor concern again.
| Generates the intervals necessary for the heat cascade diagram. Currently only works with one hot and one cold utility. | ||
| TODO: Multiple utilities. | ||
| """ | ||
| function generate_heat_cascade_intervals(prob::ClassicHENSProblem) |
There was a problem hiding this comment.
this somehow all feels unbelievably convoluted and unnecessarily complicated. But since this also seems to be only some ancillary functionality this is fine.
There was a problem hiding this comment.
The point is to make the JuMP Model()/optimization problem that defines variables on a set of intervals print nicely. Otherwise, it prints all fields and can be overwhelming/not easy to debug.
|
|
||
| Plots the cold-side composite curve. Assume intervals are already sorted e.g., attained from the `generate_heat_cascade_intervals` function. | ||
| """ | ||
| function plot_cold_composite_curve(sorted_intervals::Vector{TemperatureInterval}; ref_enthalpy = 0.0, ylabel = "T [°C or K]", xlabel = "Heat duty Q", verbose = false) |
There was a problem hiding this comment.
One could think about adding a plot recipe that adds a method to plot itself. Perhaps that is more convenient than typing this whole thing.
There was a problem hiding this comment.
Yes, I will add a recipe for a plot for the ClassicHENSProblem type, then perhaps move these to internal functionaility. TBD.
Completed:
ClassicHENSProblem- the most common kind of problem with fixed stream data