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

No deepcopy when renumbering components #916

Merged
merged 4 commits into from
Jun 23, 2024

Conversation

mtanneau
Copy link
Contributor

Motivated by #913.

This PR removes an internal deepcopy while re-indexing components in _renumber_components, thus reducing time and memory allocations.

Case #buses old time (s) in-place time (s) old memory (MiB) in-place memory (MiB)
1354_pegase 1354 0.21 0.15 29.4 20.0
2869_pegase 2869 0.60 0.2 66.7 45.7
9241_pegase 9241 2.50 1.91 208.3 136.5
13659_pegase 13659 5.44 4.63 313.0 215.6

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

prof_no_internal_deepcopy

Copy link
Member

@ccoffrin ccoffrin left a comment

Choose a reason for hiding this comment

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

For this one I cannot immediately confirm the change is ok.

The function _renumber_components does not have a bang, so it should be non-destructive for the input arguments. By changing the value of comp["index"] without copying it, some of the data in comp_dict has been changed.

So I am suspecting this function should be renamed _renumber_components! and then it should be confirmed that all existing uses are still OK when this function is destructive.

@mtanneau
Copy link
Contributor Author

That is true 🤔

I believe an in-place, destructive version would serve us better indeed. No need for a new dictionary, we can update the existing one in-place, which would also make things more explicit to the user.
Currently, we do not modify the key -> value mapping of the input dictionary directly, we modify comp_dict[i]["index"].

So I am suspecting this function should be renamed _renumber_components! and then it should be confirmed that all existing uses are still OK when this function is destructive.

I did a repo-wide search of _renumber_components, and I only found it used once, as part of make_basic_network (which already created a deepcopy of the original data dictionary). I believe changing it to a destructive version _renumber_components! would not affect code within PowerModels.
To avoid any downstream issue, I can do the following:

  • create an in-place, destructive version _renumber_components!
  • update the existing _renumber_components to be something like _renumber_components!(deepcopy(input_data))

If you want to get rid of the non-destructive version, it can be deprecated and removed in a later version.

@ccoffrin
Copy link
Member

I did a repo-wide search of _renumber_components, and I only found it used once, as part of make_basic_network (which already created a deepcopy of the original data dictionary). I believe changing it to a destructive version _renumber_components! would not affect code within PowerModels.

If that is the case, I am fine to change this to the destructive version along with the function name change. I don't mind removing the original. The change is "non-breaking" to external users because this function is indicated as internal use only and not exported.

@ccoffrin ccoffrin changed the base branch from master to release-prep June 23, 2024 19:28
@ccoffrin ccoffrin merged commit e340de8 into lanl-ansi:release-prep Jun 23, 2024
7 checks passed
@mtanneau mtanneau deleted the RenumberComps branch June 24, 2024 20:02
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