NREL-Sienna / PowerSystems.jl

Data structures in Julia to enable power systems analysis. Part of the Scalable Integrated Infrastructure Planning Initiative at the National Renewable Energy Lab.
https://www.nrel.gov/analysis/sienna.html
BSD 3-Clause "New" or "Revised" License
306 stars 74 forks source link

Document/fix surprising behaviors of `set_name!(sys, component, name)` #1161

Open GabrielKS opened 1 month ago

GabrielKS commented 1 month ago

If one renames components attached to a system in a loop over get_components:

using PowerSystems
using PowerSystemCaseBuilder

sys = build_system(PSISystems, "RTS_GMLC_DA_sys")

@show get_name.(get_components(LoadZone, sys))

for load_zone in get_components(LoadZone, sys)
    @show get_name(load_zone)
    set_name!(sys, load_zone, get_name(load_zone)*"-renamed")
end

some components will be covered multiple times:

get_name.(get_components(LoadZone, sys)) = ["11.0", "21.0", "25.0", "32.0", "33.0", "16.0", "22.0", "34.0", "26.0", "24.0", "27.0", "35.0", "12.0", "13.0", "17.0", "23.0", "36.0", "14.0", "15.0", "31.0", "37.0"]
get_name(load_zone) = "11.0"
get_name(load_zone) = "21.0"
get_name(load_zone) = "25.0"
get_name(load_zone) = "32.0"
get_name(load_zone) = "33.0"
get_name(load_zone) = "11.0-renamed"
get_name(load_zone) = "16.0"
get_name(load_zone) = "22.0"
get_name(load_zone) = "34.0"
get_name(load_zone) = "26.0"
get_name(load_zone) = "24.0"
get_name(load_zone) = "27.0"
get_name(load_zone) = "35.0"
get_name(load_zone) = "32.0-renamed"
get_name(load_zone) = "12.0"
get_name(load_zone) = "13.0"
get_name(load_zone) = "17.0"
get_name(load_zone) = "16.0-renamed"
get_name(load_zone) = "25.0-renamed"
get_name(load_zone) = "22.0-renamed"
get_name(load_zone) = "25.0-renamed-renamed"
...

because the components are stored in a dictionary indexed by name. It's reasonable to expect people to know that when iterating over a container, they're not supposed to change the container (so no adding and removing components in this loop), but I don't think it's obvious to users that set_name! changes the container, not just a field within the component (like, if this were something like set_peak_active_power!, it would be safe). @daniel-thom and I agree that it's not at all worth changing the implementation to make this behavior safe, but that we should document more explicitly that set_name! changes the container.

GabrielKS commented 1 month ago

Another surprising behavior of set_name! is that if you set a component's name to that component's existing name, it's an error:

Screenshot 2024-07-25 at 12 05 28 PM

This one I think should be changed to a no-op.

rodrigomha commented 1 day ago

I don't think we can do much here, besides adding in the documentation that every time you modify a component you should do:

for load_zone in collect(get_components(LoadZone, sys))
    @show get_name(load_zone)
    set_name!(sys, load_zone, get_name(load_zone)*"-renamed")
end

Regarding the second issue of renaming to same name, @GabrielKS can you open an issue on InfrastructureSystems?

Regarding the main issue here: @kdayday If there is some funding for documentation, I think we should add a tutorial regarding modifying data in a existing system? We can show how to increase the load of the system, or change the operating points of the generators, and we can highlight the issue of updating the name using the get_components, that you should always do collect.

kdayday commented 1 day ago

@rodrigomha yes, it is on my general to-do's to add a "manipulating data" tutorial to highlight the getters and setters, some of which was previously on other pages and is now orphaned. I'll take a note of this.

GabrielKS commented 1 day ago

@rodrigomha Opened https://github.com/NREL-Sienna/InfrastructureSystems.jl/issues/397 for the second issue.