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

Add in-place conversion to basic data format #915

Merged
merged 2 commits into from
Jun 23, 2024

Conversation

mtanneau
Copy link
Contributor

@mtanneau mtanneau commented Jun 12, 2024

Motivated by #913

This PR introduces an in-place make_basic_data!, which has the same functionality but does not create a deepcopy.
The goal is to avoid the (time and memory) cost of deepcopy-ing the initial data dictionary when the user does not want to keep it.

A typical use-case would be workflows that look like data = make_basic_network(parse_file(...)).
This call with load the raw data dictionary with parse_file, then deep-copy it, convert the copy to basic format, and discard the original raw data.

Timings of make_basic_network after the changes: times are improved, and memory consumption is reduced.
Note: those are just the ouptut of @time; I did run GC.gc() before to reduce garbage collection, but it's not a full-fledge @benchmark

Case #buses old time (s) in-place time (s) old memory (MiB) in-place memory (MiB)
1354_pegase 1354 0.21 0.13 29.4 20.4
2869_pegase 2869 0.60 0.40 66.7 48.2
9241_pegase 9241 2.50 1.87 208.3 145.9
13659_pegase 13659 5.44 4.67 313.0 227.4

and corresponding profile of make_basic_network!(data) where data = pglib("9241_pegase"):
prof_inplace

Note that the above profile does not include the improvements in #914. The main (and expected) outcome is that the time spent deep-copying at the beginning is gone.

@ccoffrin
Copy link
Member

Shall we add a test for make_basic_network! and make_basic_network that confirms that one is destructive and the other is not?

@mtanneau
Copy link
Contributor Author

I can do that. I'm currently in a plane, will push in a couple days

@ccoffrin
Copy link
Member

After that I'll be good to merge. Feel free to add your name in the list of the readme if you like.

Comment on lines +44 to +45
@test data == data_ # input data not modified
@test data_basic !== data # we created a new dict
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These ones check that we did not modify the input data, and that we returned a new Dict object


# Test #2: `make_basic_network!` is destructive and works in-place
data_basic = PowerModels.make_basic_network!(data)
@test data_copy === data # in-place modification
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I only check that the returned dictionary is exactly the one that was passed as input.
Note the use of triple equal ===

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just fixed a typo: data_copy should have been data_basic here

Comment on lines +51 to +64
# Test #3: double-check the output of `make_basic_network!`
# This may be redundant with the above tests, unless `make_basic_network` does not
# call the in-place version under the hood
@test length(data_basic["bus"]) == 4
@test length(data_basic["load"]) == 2
@test length(data_basic["shunt"]) == 2
@test length(data_basic["gen"]) == 1
@test length(data_basic["branch"]) == 2
@test length(data_basic["dcline"]) == 0
@test length(data_basic["switch"]) == 0
@test length(data_basic["storage"]) == 0

result = solve_opf(data_basic, ACPPowerModel, nlp_solver)
@test isapprox(result["objective"], 1036.52; atol=1e0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those extra tests are a safety net in case someone later decides to not call make_basic_network! inside of the non-destructive make_basic_network

@ccoffrin ccoffrin changed the base branch from master to release-prep June 23, 2024 19:27
@ccoffrin ccoffrin merged commit 8fc01e9 into lanl-ansi:release-prep Jun 23, 2024
7 checks passed
@mtanneau mtanneau deleted the InPlaceBasic branch June 24, 2024 20:03
ccoffrin added a commit that referenced this pull request Jul 4, 2024
* Improve performance of calc_connected_components (#914)

---------

Co-authored-by: mtanneau3 <[email protected]>

* Add in-place conversion to basic data format (#915)

* Add in-place conversion to basic data format

* Add unit test for in-place make_basic_network

---------

Co-authored-by: mtanneau3 <[email protected]>

* No deepcopy when renumbering components (#916)

* No deepcopy when renumbering components

* In-place _renumber_components and unit tests

* Fix typo

---------

Co-authored-by: mtanneau3 <[email protected]>
Co-authored-by: Carleton Coffrin <[email protected]>

* add notes to changelog and prep for release

* Update psse parser for 3winding transformers (#917)

* fix psse parser 3 winding tranformers
* update readme

* add note to changelog and readme

* fix quote counting check in psse parser

---------

Co-authored-by: Mathieu Tanneau <[email protected]>
Co-authored-by: mtanneau3 <[email protected]>
Co-authored-by: Rahmat Heidari <[email protected]>
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.

2 participants