Skip to content
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

Merged
merged 14 commits into from
Apr 30, 2024
Merged

Reduce time of testing #378

merged 14 commits into from
Apr 30, 2024

Conversation

zolanaj
Copy link
Collaborator

@zolanaj zolanaj commented Apr 24, 2024

No description provided.

@zolanaj zolanaj changed the base branch from master to develop April 24, 2024 03:07
@zolanaj zolanaj changed the title Do Not Merge Yet: Update test suite for speed Reduce time of testing Apr 24, 2024
@zolanaj
Copy link
Collaborator Author

zolanaj commented Apr 24, 2024

Most edits to inputs are done in runtests.jl but can all be migrated to the test .jsons

@zolanaj zolanaj requested a review from adfarth April 25, 2024 03:01
@zolanaj
Copy link
Collaborator Author

zolanaj commented Apr 29, 2024

referencing #377

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zolanaj can we remove the following uncommented line?

"# @testset "Minimize Unserved Load" is too slow with Cbc (killed after 8 hours)" (line 220)

Copy link
Collaborator Author

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)
Copy link
Collaborator

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) ?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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
Copy link
Collaborator

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 ?

Copy link
Collaborator Author

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 296bbd6

Copy link
Collaborator

@adfarth adfarth left a 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

@zolanaj zolanaj mentioned this pull request Apr 30, 2024
@zolanaj
Copy link
Collaborator Author

zolanaj commented Apr 30, 2024

@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.

@zolanaj zolanaj merged commit f99d7e7 into develop Apr 30, 2024
1 check passed
@zolanaj zolanaj deleted the simplify-tests branch April 30, 2024 20:42
@zolanaj zolanaj mentioned this pull request Apr 30, 2024
@zolanaj zolanaj mentioned this pull request May 6, 2024
indu-manogaran pushed a commit that referenced this pull request Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants