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

Refactor some of the code related to model creation #886

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

abelsiqueira
Copy link
Member

@abelsiqueira abelsiqueira commented Oct 16, 2024

  • Create new files src/utils.jl and src/model-preparation.jl to help split the code from src/create-model.jl.
  • The file src/utils.jl contains useful functions for create_model.
  • The file src/model-preparation should be useful in the future for data and structure creation that should happen before the model.
  • Create a simple sets NamedTuple for this section.
  • Create function add_expressions_to_dataframe! with a large chunk of code from create_model that adds incoming and outgoing flows to some dataframes.
  • Create function create_variables! that creates the variables in create_model.
  • Both of these functions actually involve things happening inside create_model, but for now we are keeping them there.

  • Move the sets creation to a function create_sets.
  • Store the sets in a NamedTuple

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.90%. Comparing base (fc68311) to head (090b109).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #886   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files          20       22    +2     
  Lines        1023     1046   +23     
=======================================
+ Hits         1022     1045   +23     
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abelsiqueira abelsiqueira force-pushed the 885-refactor-separate-concerns branch 2 times, most recently from ca44739 to 482a75e Compare October 16, 2024 12:43
@abelsiqueira abelsiqueira changed the title [WIP] Refactor model preparation Refactor some of the code related to model creation Oct 16, 2024
Create new files src/utils.jl and src/model-preparation.jl to help split
the code from src/create-model.jl.
The file src/utils.jl contains useful functions for create_model.
The file src/model-preparation should be useful in the future for data
and structure creation that should happen before the model.
Create a simple sets NamedTuple for this section.
Create function add_expressions_to_dataframe! with a large chunk of code
from create_model that adds incoming and outgoing flows to some
dataframes.
Create function create_variables! that creates the variables in
create_model.
Both of these functions actually involve things happening inside
create_model, but for now we are keeping them there.
@abelsiqueira abelsiqueira added the benchmark PR only - Run benchmark on PR label Oct 16, 2024
Copy link
Contributor

github-actions bot commented Oct 16, 2024

Benchmark Results

fc68311... 090b109... fc68311.../090b10958c37e2...
energy_problem/create_model 42.9 ± 1.9 s 45.9 ± 1.1 s 0.936
energy_problem/input_and_constructor 8.75 ± 0.41 s 8.82 ± 0.4 s 0.992
time_to_load 4.04 ± 0.036 s 4.05 ± 0.027 s 0.997
fc68311... 090b109... fc68311.../090b10958c37e2...
energy_problem/create_model 0.645 G allocs: 22.5 GB 0.645 G allocs: 22.5 GB 1
energy_problem/input_and_constructor 9.53 M allocs: 0.695 GB 9.53 M allocs: 0.695 GB 1
time_to_load 0.157 k allocs: 11.1 kB 0.157 k allocs: 11.1 kB 1

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@abelsiqueira
Copy link
Member Author

Copy-pasted from previous run comparing only the first commit

fc68311... ea4291f... fc68311.../ea4291f8d9269b...
energy_problem/create_model 46.8 ± 1.6 s 49.5 ± 1.9 s 0.947
energy_problem/input_and_constructor 9.27 ± 0.59 s 9.78 ± 0.34 s 0.947
time_to_load 4.31 ± 0.018 s 4.31 ± 0.073 s 1
fc68311... ea4291f... fc68311.../ea4291f8d9269b...
energy_problem/create_model 0.645 G allocs: 22.5 GB 0.645 G allocs: 22.5 GB 1
energy_problem/input_and_constructor 9.53 M allocs: 0.695 GB 9.53 M allocs: 0.695 GB 1
time_to_load 0.157 k allocs: 11.1 kB 0.157 k allocs: 11.1 kB 1

Move the sets creation to a function create_sets.
Store the sets in a NamedTuple.
@abelsiqueira
Copy link
Member Author

This doesn't close #885, but it can be merged

@abelsiqueira abelsiqueira marked this pull request as ready for review October 16, 2024 15:01
Copy link
Member

@datejada datejada left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@datejada datejada merged commit 943b7e3 into main Oct 16, 2024
7 checks passed
@datejada datejada deleted the 885-refactor-separate-concerns branch October 16, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark PR only - Run benchmark on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants