small bug fixes for PSI integration #585
Merged
Merged
Conversation
Deep review of the IS4 branch vs main (correctness, hygiene, performance) with a focused analysis of the units management layer. Findings are prioritized task cards (P1-P3) with verified evidence, acceptance criteria, decision points, and guardrails for an implementation session. https://claude.ai/code/session_01HkFivicrR478NZxtSueMSj
There was a problem hiding this comment.
Pull request overview
This PR tightens several IS4 behaviors that affect PSI/PSY integration by replacing cryptic MethodError/promotion failures with actionable ArgumentErrors, clarifying units/scaling-factor contracts, and adding regression tests around time-series-backed cost curves and relative-units arithmetic.
Changes:
- Enforce/document the 2-argument
scaling_factor_multiplier(owner, units)contract (typedunits::AbstractUnitSystem) and surface a clearArgumentErrorfor legacy 1-arg multipliers. - Improve relative-units safety and UX: reject double-tagging, throw clear errors on cross-unit and tagged-vs-untagged operations, add
hashconsistent with==, and add an unsupported-unit conversion fallback. - Fix/clarify cost-curve time-series key resolution + serialization robustness, and add tests covering round-trips and legacy unit-system decoding.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_time_series.jl | Adds regression tests for 2-arg scaling-factor multiplier contract + typed units kwarg behavior |
| test/test_supplemental_attributes.jl | Adds test for get_attr_value(::TestSupplemental) |
| test/test_relative_units.jl | Adds broader regression coverage for new relative-units error behaviors, hashing, and unsupported unit conversions |
| test/test_generate_structs.jl | Adds synthetic-descriptor test to exercise needs_conversion codegen/export behavior |
| test/test_cost_functions.jl | Adds tests for get_time_series_key behavior matrix, fallback errors, serialization round-trips, legacy unit decoding, and zero unit preservation |
| test/test_convexity_checks.jl | Adds tests ensuring is_valid_data rejects time-series-backed function data/curves with ArgumentError |
| test/InfrastructureSystemsTests.jl | Removes unused DataFramesMeta import from test runner |
| src/utils/test.jl | Fixes TestSupplemental accessor (introduces get_attr_value) |
| src/utils/generate_structs.jl | Updates needs-conversion getter docstring; clarifies _unitful export gate rationale |
| src/time_series_value_curve.jl | Refactors repeated TS function-data key resolution into _resolve_function_data_key + adds perf note to docstrings |
| src/time_series_interface.jl | Documents/annotates units::AbstractUnitSystem kwarg across accessors; rethrows legacy multiplier arity mismatch as ArgumentError |
| src/relative_units.jl | Adds double-tagging guards, clearer cross-unit/tagged-vs-untagged errors, hash, _cost_coeff_ratio fallback, and doc tweaks |
| src/production_variable_cost_curve.jl | Adds get_time_series_key fallback + FuelCurve key disambiguation; preserves unit system in zero; normalizes/validates fuel_cost deserialize |
| src/outputs.jl | Fixes stub error message to reference write_outputs |
| src/InfrastructureSystems.jl | Clarifies export policy exception for get_value/set_value generics |
| src/function_data/convexity_checks.jl | Makes is_valid_data on TS-backed data throw ArgumentError instead of MethodError |
| .claude/IS4_REVIEW.md | Adds an implementation handoff/review task file |
| .claude/claude.md | Updates repo architectural notes and documents the IS4 units/time-series contracts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## IS4 #585 +/- ##
==========================================
+ Coverage 83.51% 83.85% +0.34%
==========================================
Files 74 73 -1
Lines 6266 6301 +35
==========================================
+ Hits 5233 5284 +51
+ Misses 1033 1017 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
luke-kiernan
approved these changes
Jun 10, 2026
luke-kiernan
left a comment
luke-kiernan
left a comment
Contributor
There was a problem hiding this comment.
Lots going on and rather confusing. Would be helpful to see the bugs or issues that each of the changes was made in response to.
|
|
||
| comps = get_components(component_type, components) | ||
| data = Array{Any, 2}(undef, length(comps), length(column_labels)) | ||
|
|
Contributor
There was a problem hiding this comment.
I had to read this like 3 times to grasp the intention here. 2 problems: first, the abundance of edge cases (are these actually exercised?). Second, readability: soooo many layers of unresolved function calls and variable assignments.
| # goes through the kwarg constructor with every data key splatted, so a field | ||
| # added to the struct cannot be silently dropped: an unknown key fails loudly | ||
| # here (no `_deserialize_pvcc_field` method) or at the constructor. | ||
| _deserialize_pvcc_field(::Val{:value_curve}, raw::AbstractDict) = |
Contributor
There was a problem hiding this comment.
pvcc...oh, Point Value Cost Curve. Probably worth specifying that in a comment.
| ), | ||
| ) | ||
|
|
||
| function deserialize(::Type{FuelCurve}, data::Dict) |
Contributor
There was a problem hiding this comment.
What's the problem with the previous implementation here?
| # This has several limitations and is only a workaround for PSY.Reserve subtypes. | ||
| # - each parameter must be in _module | ||
| # - does not support nested parametrics. | ||
| # Reserves should be fixed and then we can remove this hack. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.