-
Notifications
You must be signed in to change notification settings - Fork 21
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
Reduce time of testing #378
Conversation
This reverts commit b703384.
Most edits to inputs are done in |
referencing #377 |
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 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.
done in 50c8d4b. Thanks for catching!
test/runtests.jl
Outdated
d["ElectricStorage"]["min_kwh"] = 50 | ||
d["ElectricStorage"]["max_kwh"] = 50 | ||
d["Financial"]["microgrid_upgrade_cost_fraction"] = 0.0 | ||
s = Scenario(d) |
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.
@zolanaj just curious, was there a compute time advantage to creating the Scenario and REoptInputs first, rather than just running run_reopt(m,d)
?
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 don't think there's any difference in computing - run_reopt(m, d) does the same sequence of commands. I've always used the Scenario and REoptInputs creation upfront for debugging purposes; it's fewer lines of code in the testing though so I can convert it.
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.
done in 6747804 for the updated tests only; we can probably do this more widely but I don't see any reason for a computational difference so that's low priority.
test/runtests.jl
Outdated
m = Model(optimizer_with_attributes(HiGHS.Optimizer, "output_flag" => false, "log_to_console" => false, "mip_rel_gap" => 0.01, "presolve" => "on")) | ||
results = run_reopt(m, p) | ||
@test value(m[:binMGTechUsed]["PV"]) ≈ 0 | ||
@test sum(results["Outages"]["unserved_load_per_outage_kwh"]) > 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.
@zolanaj any reason to not set this to the value of 24.16 instead of just > 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.
I think this one is stable enough to make 24.16. I usually stay away from fixed values when a 1% mip tolerance is involved, but I think this one should be ok fo now. Good catch!
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.
done in 296bbd6
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.
@zolanaj this looks good and it's great to have the tests passing in ~an hour rather than 3!! I am approving but also have a question and suggestion (and a few more minor qs in the comments):
- Should we definitely remove ubuntu from from the test os's?
- I do think it would make sense to pull your changes into the post jsons, as you've suggested
Thank you for all of this @adfarth ! I have an issue here for the Ubuntu tests and implemented the suggested edits with links to specific commits in the replies. Once tests are confirmed to pass, I'll merge into develop. |
Reduce time of testing
No description provided.