Review codebase and identify duplications #1580

jd-lara · 2025-11-11T07:41:20Z

Thanks for opening a PR to PowerSystems.jl, please take note of the following when making a PR:

Add a description of the changes proposed in the pull request.
Is my PR fixing an open issue? Add the reference to the related issue
If you are contributing a new struct: please refer to the testing requirements for new structs

Detailed analysis identifying: - Code duplications in validation, service management, and accessors - Performance improvements (type stability, allocations, field access) - Precompilation assessment (clean - no barriers found) Key findings: - 1 high-priority type instability issue in parser - 8 medium-priority code duplications and optimizations - 6 low-priority cleanup opportunities Report includes specific file locations, line numbers, code examples, and prioritized recommendations for improvements.

…tion This commit addresses high-priority performance issues identified in the codebase review report (CODEBASE_REVIEW_REPORT.md). Changes: 1. Fix type instability in _FieldInfo struct (HIGH PRIORITY) - Changed default_value from ::Any to Union{Float64, Int, String} - Enables type inference throughout parser code path - File: src/parsers/power_system_table_data.jl:1746 2. Remove unnecessary collect(keys(...)) calls - Eliminated 5 unnecessary array allocations - sort() and findmin() work directly with key iterators - Files: * src/utils/IO/branchdata_checks.jl (2 occurrences) * src/parsers/pm_io/psse.jl (3 occurrences) - Note: Kept collect() in base.jl:2656 for public API compatibility 3. Replace maximum([a,b]) with max(a,b) - Eliminates unnecessary array allocation - File: src/utils/IO/branchdata_checks.jl:194 4. Refactor getfield/getproperty loops for better type specialization - Created helper functions to validate/correct rating fields - Eliminates dynamic field access that prevents type inference - Direct field access allows compiler specialization - Files: src/utils/IO/branchdata_checks.jl * _validate_line_rating_field() * _correct_single_rate_limit!() * _validate_transformer_rating_field() - Refactored 3 functions: * check_rating_values (Line/MonitoredLine) * correct_rate_limits! * check_rating_values (TwoWindingTransformer) Performance Impact: - Reduced allocations in validation hot paths - Improved type inference in parser infrastructure - Better compiler specialization for branch data checks - No behavioral changes, purely optimization All changes maintain existing logic and error handling behavior.

github-actions · 2025-11-11T07:42:47Z

src/utils/IO/branchdata_checks.jl

+    _validate_line_rating_field(:rating, line.rating, line_name, basemva, closest_v_level, closest_rate_range)
+    _validate_line_rating_field(:rating_b, line.rating_b, line_name, basemva, closest_v_level, closest_rate_range)
+    _validate_line_rating_field(:rating_c, line.rating_c, line_name, basemva, closest_v_level, closest_rate_range)


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

_validate_line_rating_field(:rating, line.rating, line_name, basemva, closest_v_level, closest_rate_range)

_validate_line_rating_field(:rating_b, line.rating_b, line_name, basemva, closest_v_level, closest_rate_range)

_validate_line_rating_field(:rating_c, line.rating_c, line_name, basemva, closest_v_level, closest_rate_range)

_validate_line_rating_field(

:rating,

line.rating,

line_name,

basemva,

closest_v_level,

closest_rate_range,

)

_validate_line_rating_field(

:rating_b,

line.rating_b,

line_name,

basemva,

closest_v_level,

closest_rate_range,

)

_validate_line_rating_field(

:rating_c,

line.rating_c,

line_name,

basemva,

closest_v_level,

closest_rate_range,

)

Copilot

Pull Request Overview

This PR implements code quality improvements identified in a comprehensive codebase review, focusing on reducing code duplication and improving performance in the PowerSystems.jl library.

Key changes:

Refactored duplicated rating validation logic into reusable helper functions
Replaced collect(keys(...)) calls with direct key iteration to reduce allocations
Narrowed type specification for _FieldInfo.default_value from Any to Union{Float64, Int, String}

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`CODEBASE_REVIEW_REPORT.md`	New comprehensive review document identifying 9 high-priority issues and 15+ optimization opportunities
`src/utils/IO/branchdata_checks.jl`	Extracted duplicated validation logic into helper functions `_validate_line_rating_field`, `_validate_transformer_rating_field`, and `_correct_single_rate_limit!`; removed unnecessary `collect()` calls on dictionary keys
`src/parsers/power_system_table_data.jl`	Changed `default_value` field type from `Any` to `Union{Float64, Int, String}` for improved type stability
`src/parsers/pm_io/psse.jl`	Removed unnecessary `collect()` calls when sorting dictionary keys to reduce allocations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/utils/IO/branchdata_checks.jl

src/parsers/pm_io/psse.jl

Copilot · 2025-11-11T07:44:54Z

src/parsers/power_system_table_data.jl

    }
    unit_conversion::Union{NamedTuple{(:From, :To), Tuple{String, String}}, Nothing}
-    default_value::Any
+    default_value::Union{Float64, Int, String}


The type constraint change from Any to Union{Float64, Int, String} may be too restrictive. The code shows default_value can be:

String literals like "required" (line 1785, 1840)

String literals like "system_base_power" which gets replaced with data.base_power (line 1786-1787)

Other values from YAML descriptors (line 1785)

Without comprehensive knowledge of all possible values in the YAML descriptor files, this change could cause runtime errors if the descriptors contain other types (e.g., Nothing, Bool, other numeric types). Consider either:

Keeping Any if the type variety is truly unknown

Using Union{Float64, Int, String, Nothing} if Nothing is possible

Adding validation to ensure only these types are present in descriptors

The original report suggests this improves type stability, but it could introduce breaking changes if the constraint is too narrow.

Suggested change

default_value::Union{Float64, Int, String}

default_value::Union{Float64, Int, String, Nothing}

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

This commit reverts the invalid optimization that attempted to remove collect() calls on dictionary keys. The original commit incorrectly assumed that Julia's sort() and findmin() operations could work directly on KeySet iterators. Issues with the incorrect optimization: 1. In branchdata_checks.jl: - keys(dict) returns a KeySet, not an indexable array - findmin(generator) returns (value, index) where index is an iteration position, not a dictionary key - Cannot index into a KeySet with keySet[index] - The collect() is necessary to convert KeySet to an array for indexing 2. In psse.jl: - sort() requires a concrete collection, not a KeySet iterator - The collect() is necessary to convert the iterator to an array Restored (correct) code: - collect(keys(...)) in branchdata_checks.jl (2 locations) - collect(keys(...)) in psse.jl (3 locations) Valid optimizations kept: ✅ Type stability fix: default_value::Union{Float64, Int, String} ✅ Replace maximum([a,b]) with max(a,b) ✅ Refactor getfield() loops with helper functions Thank you for catching this error!

Updated CODEBASE_REVIEW_REPORT.md to reflect that the collect() removal suggestion (section 2.2) was incorrect and has been reverted. Changes to report: - Added correction warning at top - Updated executive summary with corrected counts - Section 2.2: Marked as INVALID with detailed explanation - Updated recommendations to mark item #2 as invalid - Marked implemented fixes with ✅ checkmarks: * Item #1: Type stability fix (DONE) * Item #6: Field access refactoring (DONE) * Item #8: max() optimization (DONE) This ensures future readers understand the limitation and don't attempt the invalid optimization.

The helper function was receiving both a field name and the field's value as separate parameters. This created a confusing design where: - The value parameter could become stale if the field was modified - It was unclear whether checks operated on the passed value or current field - The function checked rating_value but used setfield! on field_name Fixed by having the helper access the field directly via getfield: - Removed rating_value parameter - Added getfield(branch, field_name) at start of function - Now there's a single source of truth for the field value - Clearer semantics: function reads current value, checks it, may modify it The validation helpers (_validate_line_rating_field and _validate_transformer_rating_field) are unchanged because they only read values for logging/validation without modification, so passing the value is appropriate and avoids unnecessary getfield calls. Thank you for catching this design issue!

github-actions · 2025-11-20T01:33:59Z

src/utils/IO/branchdata_checks.jl

+    _validate_transformer_rating_field(:rating, xfrm.rating, xfrm_name, device_base_power, closest_v_level, closest_rate_range)
+    _validate_transformer_rating_field(:rating_b, xfrm.rating_b, xfrm_name, device_base_power, closest_v_level, closest_rate_range)
+    _validate_transformer_rating_field(:rating_c, xfrm.rating_c, xfrm_name, device_base_power, closest_v_level, closest_rate_range)


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

_validate_transformer_rating_field(:rating, xfrm.rating, xfrm_name, device_base_power, closest_v_level, closest_rate_range)

_validate_transformer_rating_field(:rating_b, xfrm.rating_b, xfrm_name, device_base_power, closest_v_level, closest_rate_range)

_validate_transformer_rating_field(:rating_c, xfrm.rating_c, xfrm_name, device_base_power, closest_v_level, closest_rate_range)

_validate_transformer_rating_field(

:rating,

xfrm.rating,

xfrm_name,

device_base_power,

closest_v_level,

closest_rate_range,

)

_validate_transformer_rating_field(

:rating_b,

xfrm.rating_b,

xfrm_name,

device_base_power,

closest_v_level,

closest_rate_range,

)

_validate_transformer_rating_field(

:rating_c,

xfrm.rating_c,

xfrm_name,

device_base_power,

closest_v_level,

closest_rate_range,

)

Replaced runtime type checking with multiple dispatch to enable better type inference and compiler specialization in PSSE dynamic data parser. Before (runtime dispatch with isa() checks): function _parse_input_types(_v, val) if isa(_v, Float64) # handle Float64 elseif isa(_v, Int) # handle Int elseif isa(_v, String) # handle String else error(...) end end After (compile-time dispatch): _parse_input_types(v::Float64, val) = ... _parse_input_types(v::Int, val) = ... _parse_input_types(v::String, val) = ... _parse_input_types(v, val) = error(...) # fallback Benefits: - Type specialization: Compiler generates optimized code for each type - Better type inference: Each method has clear input/output types - Eliminates runtime isa() checks: Dispatch happens at compile time - More idiomatic Julia: Uses the language's strength (multiple dispatch) - Better error messages: Fallback shows the unexpected type No behavioral changes - all logic preserved, just refactored for performance. Addresses recommendation 2.4 from codebase review report.

…compile Refactored three functions with runtime type checking (isa()) to use multiple dispatch for better type stability and compiler optimization. Changes: 1. orderedlimits in base_checks.jl: - Split into two methods: one for Nothing, one for NamedTuple - Eliminates runtime isa(limits, Nothing) check - Compiler can now specialize for each case 2. check_type in matlab.jl: - Split into three methods based on value type - Method 1: Value already correct type (T) - Method 2: String/SubString → parse to type - Method 3: Fallback for type conversion - Eliminates multiple isa() checks in single function 3. _parse_bool_value helper in power_system_table_data.jl: - New helper using multiple dispatch - Method 1: Bool → return as-is - Method 2: Other types → parse to Bool - Replaces if !isa(gen_must_run, Bool) pattern 4. Removed obsolete __precompile__() declaration: - Removed isdefined(Base, :__precompile__) && __precompile__() - In Julia 1.0+, modules are precompiled by default - This was Julia 0.6 compatibility code Benefits: - Better type inference in all three functions - Compile-time dispatch instead of runtime type checking - More idiomatic Julia code - Improved performance potential through specialization - Cleaner, more maintainable code Addresses recommendations 2.4 and 10 from codebase review report.

…cess Replaced unnecessary getfield() and getproperty() calls with direct field access when the field name is known at compile time. Changes: 1. branchdata_checks.jl line 257: Before: x_pu = getproperty(xfrm, :x) After: x_pu = xfrm.x 2. power_system_table_data.jl line 381: Before: getfield(data, :timeseries_metadata_file) After: data.timeseries_metadata_file Rationale: - getfield/getproperty with literal symbols adds unnecessary indirection - Direct field access (obj.field) is: * More idiomatic and readable * Potentially better optimized by compiler * Clearer intent when field is compile-time constant Note: getfield(branch, field_name) remains in helper function where field_name is a runtime-determined variable - this is the correct usage. Guideline established: - Compile-time known field: use direct access (obj.field) - Runtime-determined field: use getfield(obj, field_symbol) Addresses recommendation 9 from codebase review report.

jd-lara · 2025-11-22T01:32:51Z

most of the changes are for the parsers which are due to depart this repository. I'll close

claude added 2 commits November 11, 2025 06:34

jd-lara requested a review from Copilot November 11, 2025 07:41

Copilot started reviewing on behalf of jd-lara November 11, 2025 07:42 View session

github-actions bot reviewed Nov 11, 2025

View reviewed changes

Copilot finished reviewing on behalf of jd-lara November 11, 2025 07:44

Copilot AI reviewed Nov 11, 2025

View reviewed changes

jd-lara and others added 4 commits November 19, 2025 18:18

Apply suggestion from @Copilot

154e3d1

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

github-actions bot reviewed Nov 20, 2025

View reviewed changes

claude added 3 commits November 20, 2025 01:40

jd-lara closed this Nov 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Review codebase and identify duplications #1580

Review codebase and identify duplications #1580

Uh oh!

jd-lara commented Nov 11, 2025

Uh oh!

github-actions bot Nov 11, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 11, 2025

Uh oh!

github-actions bot Nov 20, 2025

Uh oh!

jd-lara commented Nov 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	default_value::Union{Float64, Int, String}
	default_value::Union{Float64, Int, String, Nothing}

Review codebase and identify duplications #1580

Review codebase and identify duplications #1580

Uh oh!

Conversation

jd-lara commented Nov 11, 2025

Uh oh!

github-actions bot Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

jd-lara commented Nov 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants