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

Close multiple simple issues in PSY #1192

Merged
merged 11 commits into from
Sep 16, 2024
Merged

Close multiple simple issues in PSY #1192

merged 11 commits into from
Sep 16, 2024

Conversation

rodrigomha
Copy link
Contributor

@rodrigomha rodrigomha commented Sep 13, 2024

Closes #1175 via 692dfe0
Closes #1165 via 9dd3ef2
Closes #1162 via a62741c
Closes #1147 via 676e777
Closes #1160 via d954dee

@rodrigomha rodrigomha self-assigned this Sep 13, 2024
@@ -13,7 +13,6 @@ The `PowerSystems.jl` package provides a rigorous data model using Julia structu
## Version Advisory

- PowerSystems will work with Julia v1.6+.
- If you are planning to use `PowerSystems.jl` in your package, check the [roadmap to version 4.0](https://github.com/NREL-Sienna/PowerSystems.jl/projects/4) for upcoming changes
Copy link
Member

Choose a reason for hiding this comment

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

I need to make the version 5.0

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.63%. Comparing base (d954dee) to head (7fc254a).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/models/generated/MonitoredLine.jl 66.66% 2 Missing ⚠️
src/utils/print.jl 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1192   +/-   ##
=======================================
  Coverage   84.63%   84.63%           
=======================================
  Files         180      180           
  Lines        8264     8264           
=======================================
  Hits         6994     6994           
  Misses       1270     1270           
Flag Coverage Δ
unittests 84.63% <77.77%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/base.jl 89.34% <ø> (ø)
src/models/generated/Line.jl 100.00% <100.00%> (ø)
src/parsers/power_models_data.jl 91.98% <100.00%> (ø)
src/utils/IO/system_checks.jl 83.82% <100.00%> (ø)
src/models/generated/MonitoredLine.jl 94.28% <66.66%> (ø)
src/utils/print.jl 82.14% <0.00%> (ø)

src/parsers/pm_io/data.jl Show resolved Hide resolved
@@ -346,9 +345,15 @@ function read_loadzones!(
load_zone_map[zone]["pd"] += get(load, "py", 0.0)
load_zone_map[zone]["qd"] += get(load, "qy", 0.0)
end

default_loadzone_naming = x -> string(x)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
default_loadzone_naming = x -> string(x)
default_loadzone_naming = string

Copy link
Member

Choose a reason for hiding this comment

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

good catch


default_loadzone_naming = x -> string(x)
# The formatter for loadzone_name should be a function that transform the LoadZone Int to a String
_get_name = get(kwargs, :loadzone_name_formatter, default_loadzone_naming)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get unit tests for this?

@GabrielKS GabrielKS self-requested a review September 16, 2024 21:06
"psse_Benchmark_4ger_33_2015_sys";
loadzone_name_formatter = x -> string(3 * x),
)
lz_original = first(get_components(LoadZone, sys2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you can safely assume that the result of get_components will be in the same order every time. Maybe get all the LoadZone names in each, sort those lists, and compare the lists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only one load zone for that system, that's why I can use first

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh okay

"psse_Benchmark_4ger_33_2015_sys";
loadzone_name_formatter = x -> string(3 * x),
)
lz_original = first(get_components(LoadZone, sys2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick:

Suggested change
lz_original = first(get_components(LoadZone, sys2))
lz_original = only(get_components(LoadZone, sys2))

loadzone_name_formatter = x -> string(3 * x),
)
lz_original = first(get_components(LoadZone, sys2))
lz_new = first(get_components(LoadZone, sys3))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
lz_new = first(get_components(LoadZone, sys3))
lz_new = only(get_components(LoadZone, sys3))

Copy link
Collaborator

@GabrielKS GabrielKS left a comment

Choose a reason for hiding this comment

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

Looks good!

@jd-lara jd-lara merged commit 653a425 into main Sep 16, 2024
7 of 8 checks passed
@jd-lara jd-lara deleted the rh/fix_multiple_issues branch December 13, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants