-
Notifications
You must be signed in to change notification settings - Fork 4
Remove global variables and use PrecompileTools #9
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
Conversation
|
I think there's still 10-100X in performance lying around:
JuMP.@constraint(
model,
EQG_COMBAL[(r, t, c, s) in indices["EQG_COMBAL"]],
(
!isnothing(data[:RHS_COMPRD]) && ((r, t, c, s) in data[:RHS_COMPRD]) ? ComPrd[(r, t, c, s)] :
(
sum(
(
(r, p, c) in data[:RPC_STG] ?
sum(
sum(
StgFlo[(r, v, t, p, c, ts, "OUT")] *
get(data[:RS_FR], (r, s, ts), 0) *
(
1 + (
!isnothing(data[:RTCS_FR]) ?
get(data[:RTCS_FR], (r, t, c, s, ts), 0) : 0
)
) *
data[:STG_EFF][r, v, p] for v in get(sets.RTP_VNT, (r, t, p), Set())
) for ts in sets.RPC_TS[r, p, c]
) :
sum(
sum(
PrcFlo[(r, v, t, p, c, ts)] *
get(data[:RS_FR], (r, s, ts), 0) *
(
1 + (
!isnothing(data[:RTCS_FR]) ?
get(data[:RTCS_FR], (r, t, c, s, ts), 0) : 0
)
) for v in get(sets.RTP_VNT, (r, t, p), Set());
init = 0,
) for ts in sets.RPC_TS[r, p, c]
)
) for p in get(sets.RCIO_P, (r, c, "OUT"), Set()) if (r, t, p) in data[:RTP_VARA];
init = 0,
) + sum(
sum(
sum(
IreFlo[(r, v, t, p, c, ts, "IMP")] *
get(data[:RS_FR], (r, s, ts), 0) *
(
1 + (
!isnothing(data[:RTCS_FR]) ?
get(data[:RTCS_FR], (r, t, c, s, ts), 0) : 0
)
) for v in get(sets.RTP_VNT, (r, t, p), Set());
init = 0,
) for ts in sets.RPC_TS[r, p, c]
) for p in get(sets.RCIE_P, (r, c, "IMP"), Set()) if (r, t, p) in data[:RTP_VARA];
init = 0,
)
) * data[:COM_IE][r, t, c, s]
) >=
sum(
(
(r, p, c) in data[:RPC_STG] ?
sum(
sum(
StgFlo[(r, v, t, p, c, ts, "IN")] *
get(data[:RS_FR], (r, s, ts), 0) *
(1 + (!isnothing(data[:RTCS_FR]) ? get(data[:RTCS_FR], (r, t, c, s, ts), 0) : 0))
for v in get(sets.RTP_VNT, (r, t, p), Set());
init = 0,
) for ts in sets.RPC_TS[r, p, c]
) :
(sum(
sum(
PrcFlo[(r, v, t, p, c, ts)] *
get(data[:RS_FR], (r, s, ts), 0) *
(1 + (!isnothing(data[:RTCS_FR]) ? get(data[:RTCS_FR], (r, t, c, s, ts), 0) : 0))
for v in get(sets.RTP_VNT, (r, t, p), Set());
init = 0,
) for ts in sets.RPC_TS[r, p, c]
))
) for p in get(sets.RCIO_P, (r, c, "IN"), Set()) if (r, t, p) in data[:RTP_VARA];
init = 0,
) +
sum(
sum(
sum(
IreFlo[(r, v, t, p, c, ts, "EXP")] *
get(data[:RS_FR], (r, s, ts), 0) *
(1 + (!isnothing(data[:RTCS_FR]) ? get(data[:RTCS_FR], (r, t, c, s, ts), 0) : 0)) for
v in get(sets.RTP_VNT, (r, t, p), Set());
init = 0,
) for ts in sets.RPC_TS[r, p, c]
) for p in get(sets.RCIE_P, (r, c, "EXP"), Set()) if (r, t, p) in data[:RTP_VARA];
init = 0,
) +
get(data[:COM_PROJ], (r, t, c), 0) * data[:COM_FR][r, t, c, s]
) |
|
Thanks a lot @odow! I have not gone through all the changes, but to answer your comments/questions briefly:
What could that data structure be? Please bare in mind that the data that TIMES source code receives is different from the one used here, because the prototype does not include preprocessing. Or did you mean a data structure for the internal sets / parameters?
Here you go: https://github.com/etsap-TIMES/TIMES_model/blob/master/eqcombal.mod |
|
|
||
| [deps] | ||
| DataFrames = "a93c6f00-e57d-5684-b7b6-d8193f3e46c0" | ||
| GAMS = "1ca51c6a-1b4d-4546-9ae1-53e0a243ab12" |
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 should definitely keep this one to enable the users to use the solvers they may have through their GAMS license.
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.
No, this should be removed from TIMES.jl. The user can separately do ] add GAMS and then set_optimizer(model, GAMS.Optimizer). There's no need to force every person to install GAMS.jl
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 understand your point, but the rationale for including it would be similar to HiGHs. Many of the TIMES users have access to solvers through their perpetual GAMS licenses, so it should be easy for them to try this out.
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.
These ones should actually be sets (i.e unique elements, order doesn't matter, etc...). Why to change them to arrays?
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 can revert if you like. I would have assumed that the input database did not accept duplicate values. If so, that seems like it's either a sanity check somewhere else, or an error here.
Also note that Set{String} is implemented as a Dict{String,Nothing}, so there is some cost as opposed to Vector{String}. It depends how large the list of elements is. Fewer than 100 and it's probably better to use Vector.
| if row_number > 0 && col_number == 1 | ||
| # One-dimensional set | ||
| value = Set(values(df[!, 1])) | ||
| return values(df[!, 1]) |
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 believe this should be a set, i.e. elements should be unique.
| return Dict(Tuple.(eachrow(df[:, DataFrames.Not(:value)])) .=> df.value) | ||
| else | ||
| value = Set(Tuple.(eachrow(df))) | ||
| return Tuple.(eachrow(df)) |
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.
Also here, the tuples should be unique.
|
Interesting! When I run the code twice (by executing |
|
|
||
| PrecompileTools.@setup_workload begin | ||
| PrecompileTools.@compile_workload begin | ||
| create_model("PROTO.db3") |
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 this have a desired effect also when "PROTO.db3" changes?
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 does't matter if the user's .db3 is different to PROTO.db3. All that matters is that PROTO.db3 is a small representative file that hits the correct code paths. You could make it even smaller.
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.
Ok, understood. How about this one: https://julialang.github.io/PackageCompiler.jl?
Majority of the TIMES users do not interact with the code directly, but mostly focus on the input data (thanks to a very good separation of data and structure from the code). Would this be a good way to distribute the code for those users?
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 wouldn't worry about how to distribute code yet. Let's assume you're >1 year away from delivering an end-user product. I assume the first users will be more technically savvy. They can manually install Julia.
For future, yes, you could do something like PackageCompiler, but there are number of gotchas. It wouldn't encourage you to spend time on this just yet. There's also a push within the core Julia developers to improve static compilation of binaries and apps like this. I would expect the landscape in a year to be different to today.
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 goal of the project is to provide guidance to the community on the benefits and barriers to a possible migration to Julia/JuMP. To be useful, it should take into account how the TIMES source code is used and distributed at the moment. That is why it is an important question. It is good to know that things are being developed. 💪
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 lot of work happening on juliac which aims at producing a binary from a Julia source code, but it's still in early stage of development. An essential trim feature will appear in Julia v1.12.
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.
Thanks @blegat! It is really good to know where things are headed.
Yes. But it's also an artifact of the tiny problem size. For larger models, you'd probably see a difference between the two formulations. |
That is actually horrific. I don't understand how it is possible to read or test that. |
siddharth-krishna
left a comment
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.
Thanks Oscar, not a Julia programmer, but at a high level your changes look good to me. Very impressive speedup, and reduction in memory!
What data structure would you recommend for reading the input into?
|
I would keep as much of the data munging in the DB as possible. Extract only the data that you need when you need it. Do all the filtering and joining when needed. See, for example: |
:-) Will be aiming at getting a larger problem soon! |
Why so? Is it not much slower than in-memory operations? |
This version may be more comprehensible! |
Performance is not the main reason. You can write equally fast normal Julia code. Main reasons are:
You may want to look at https://github.com/TulipaEnergy/TulipaEnergyModel.jl/tree/main/src for inspiration. |
I attempted to make a fairly minimal set of changes.
, but there's a difference between the two models I need to debug.Before
This PR