Pin demand aggregation keyword behavior (#126) - #2
Open
PabloBotin wants to merge 1 commit into
Open
Conversation
`plot_demand`/`plot_demand!` accept both the string `aggregate` alias and PowerAnalytics' typed `aggregation` keyword, but neither spelling was pinned by a test and the docstrings only mentioned the string form. Add unit tests over `_translate_demand_aggregate` covering the string table, the typed passthrough that issue Sienna-Platform#126 asked for, `aggregate = nothing`, and the `ArgumentError` on an unknown string, plus an end-to-end plot assertion that `aggregation = ACBus` survives to `get_load_data`. Document `aggregation::Type` as the primary spelling with the types PowerAnalytics actually accepts, keep `aggregate::String` as the alias, note that it is a no-op for `Results`, and correct `Bus` to `ACBus`.
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.
Closes Sienna-Platform#126.
Context
Issue Sienna-Platform#126 reports that
plot_demand/plot_demand!document anaggregatekeyword that is not actually used, while PowerAnalytics expects a typedaggregation.Investigating on
main, the functional gap is already closed:_translate_demand_aggregaterewritesaggregate(string or type) intoaggregation, and a directly-suppliedaggregationpasses through untouched. What was missing is that neither spelling was pinned by a test, and the docstrings still documented only the string form — which is how the mismatch went unnoticed in the first place.So this is a test-and-docs change;
src/behavior is unchanged.Changes
test/test_demand_aggregation.jl(new) — unit tests over_translate_demand_aggregate: all three string mappings (with:aggregateremoved and:aggregationset), the typed passthrough issue Addaggregationkeyword argument toplot_demandandplot_demand!Sienna-Platform/PowerGraphics.jl#126 asked about, a type supplied under the legacyaggregatename,aggregate = nothingas a no-op, and theArgumentErroron an unknown string. A final assertion checks every mapped type falls inside the setPA.get_load_dataaggregates over, so a PowerAnalytics signature change is caught rather than silently ignored.test/test_plot_creation.jl— end-to-end pin thataggregation = ACBussurvives toget_load_data. Reuses the existingsys_with_ts, passes nosave, so the expected-file assertion below it is unaffected.src/call_plots.jl— docstrings only.aggregation::Typeis documented as the primary spelling with the four forms PowerAnalytics accepts,aggregate::Stringas the convenience alias, and both now note it only applies when plotting aSystem(PowerAnalytics silently ignores it forResults). CorrectsPowerSystems.BustoPowerSystems.ACBus.Verification
52 passed / 52 total, exit 0, zero Error log events._translate_demand_aggregatemakes the new test file fail (1 failed, 4 errored), confirming the tests are load-bearing.Known limitations
plot_length == 3, the same count as the pre-existingaggregate = "Bus"case; it proves the typed keyword survives the translation path but is not independently load-bearing for detecting a dropped keyword — the existingaggregate = "System"case (expects 1) carries that signal."PowerLoad"is covered by the type-set assertion but not end to end.DocumenterInterLinksinventory fetches fail on a self-signed cert in this environment; pristineHEADfails identically). The@extreftargets added are all already used elsewhere in the same file, but CI should confirm.