-
Notifications
You must be signed in to change notification settings - Fork 4
Build against component-graph v0.6.0 #91
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ class ComponentGraphConfig: | |
| allow_unspecified_inverters: bool = False, | ||
| disable_fallback_components: bool = False, | ||
| include_phantom_loads_in_consumer_formula: bool = False, | ||
| prefer_meters_in_component_formulas: bool = True, | ||
| prefer_meters_in_component_formulas: bool = False, | ||
| formula_overrides: FormulaOverrides | None = None, | ||
| ) -> None: | ||
| """Initialize this instance. | ||
|
|
@@ -49,26 +49,31 @@ class ComponentGraphConfig: | |
| the measurements of successor meters from the measurements of their | ||
| predecessor meters. When `false`, consumer formula is generated by | ||
| excluding production and battery components from the grid measurements. | ||
| The non-consumer components behind one internal meter are excluded | ||
| as one group: the meter reading first, the component readings as | ||
| the fallback. While the meter reading is used, a load behind | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Docstring: meter-subtraction note under the "when false" branch reads as contradictory This grouping description is placed inside the This is the exact subtlety the release note calls out (RELEASE_NOTES.md:15), and it is accurate, but as written under the "when false" bullet it reads as self-contradictory and is easy to misread. ImpactReaders configuring Suggested fixReword to make explicit that this is a side effect of grouping-by-meter (the meter reading is used as the primary term, so anything behind that meter — including an off-graph load — is captured by that single term), not the phantom-load feature being partially enabled. 🔵 Low Severity: documentation clarity only. |
||
| that meter that is not in the component graph is then also | ||
| excluded together with the group. | ||
| prefer_meters_in_component_formulas: Default policy for the per-category | ||
| formulas. When `True` (the default), the meter measurement is the | ||
| primary source and the device measurement is the fallback for | ||
| formulas. When `False` (the default), the component measurement is the | ||
| primary source and the meter measurement is the fallback for | ||
| `battery_formula`, `chp_formula`, `pv_formula`, `wind_turbine_formula`, | ||
| `ev_charger_formula`, and `steam_boiler_formula`. When `False`, the | ||
| device is primary and the meter is the fallback. Has no effect on | ||
| `ev_charger_formula`, and `steam_boiler_formula`. When `True`, the | ||
| meter is primary and the component is the fallback. Has no effect on | ||
| `grid_formula`, `consumer_formula`, `producer_formula`, or any of the | ||
| coalesce formulas. | ||
| formula_overrides: Per-formula overrides for the meter/device preference; | ||
| formula_overrides: Per-formula overrides for the meter/component preference; | ||
| see `FormulaOverrides`. Each entry, when set, takes precedence over | ||
| `prefer_meters_in_component_formulas` for that formula. | ||
| """ | ||
|
|
||
| class FormulaOverrides: | ||
| """Per-formula overrides for the meter/device preference. | ||
| """Per-formula overrides for the meter/component preference. | ||
|
|
||
| Each parameter is `None` by default, meaning the corresponding formula | ||
| follows the global `prefer_meters_in_component_formulas` setting on | ||
| `ComponentGraphConfig`. Setting `True` forces the meter as primary | ||
| for that formula; `False` forces the device. | ||
| for that formula; `False` forces the component. | ||
| """ | ||
|
|
||
| def __init__( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,10 +141,11 @@ def test_wind_turbine_graph() -> None: | |
| } | ||
|
|
||
| # 3. Test Formula Generation | ||
| # References the Meter (ID 2) measuring the Turbine (ID 3). | ||
| # Component-first by default: the turbine (ID 3) is the primary source, | ||
| # the meter (ID 2) the fallback. | ||
| assert ( | ||
| graph.wind_turbine_formula(wind_turbine_ids={ComponentId(3)}) | ||
| == "COALESCE(#2, #3, 0.0)" | ||
| == "COALESCE(#3, #2, 0.0)" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Broken intermediate commit: default-flip test expectations land before the flip Commit At
The two files directly contradict each other and the test suite is red at Impact
Related locations
Suggested fixMove the wind/steam expectation updates (this hunk and the steam one) into the default-flip commit 🟡 Medium Severity: does not affect the merged result, but breaks bisectability and the commit message is inaccurate; best fixed before merge unless the PR is squash-merged. |
||
| ) | ||
|
|
||
| # 4. Test Topology (Successors/Predecessors) | ||
|
|
@@ -179,10 +180,43 @@ def test_steam_boiler_graph() -> None: | |
| } | ||
| assert ( | ||
| graph.steam_boiler_formula(steam_boiler_ids={ComponentId(3)}) | ||
| == "COALESCE(#2, #3, 0.0)" | ||
| == "COALESCE(#3, #2, 0.0)" | ||
| ) | ||
|
|
||
|
|
||
| def test_consumer_formula_meter_subtraction() -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 This new test pins the clean grouping ImpactThe most user-visible risk of the v0.6.0 rewrite (a phantom load being netted into a producer group) is undocumented by tests, so a future regression in that grouping would pass CI. Suggested testsAdd a case where meter 🔵 Low Severity: additional edge-case coverage for the headline behavior of this release. |
||
| """Test the consumer formula's meter-subtraction grouping. | ||
|
|
||
| The non-consumer components behind one internal meter are subtracted | ||
| as one group: the meter reading first, the component readings as the | ||
| fallback. | ||
| """ | ||
| graph: microgrid_component_graph.ComponentGraph[ | ||
| Component, ComponentConnection, ComponentId | ||
| ] = microgrid_component_graph.ComponentGraph( | ||
| components={ | ||
| GridConnectionPoint( | ||
| id=ComponentId(1), | ||
| microgrid_id=MicrogridId(1), | ||
| rated_fuse_current=100, | ||
| ), | ||
| Meter(id=ComponentId(2), microgrid_id=MicrogridId(1)), | ||
| Meter(id=ComponentId(3), microgrid_id=MicrogridId(1)), | ||
| SolarInverter(id=ComponentId(4), microgrid_id=MicrogridId(1)), | ||
| }, | ||
| connections={ | ||
| # Grid -> Grid Meter -> PV Meter -> PV Inverter | ||
| ComponentConnection(source=ComponentId(1), destination=ComponentId(2)), | ||
| ComponentConnection(source=ComponentId(2), destination=ComponentId(3)), | ||
| ComponentConnection(source=ComponentId(3), destination=ComponentId(4)), | ||
| }, | ||
| ) | ||
|
|
||
| # The PV group behind meter #3 is subtracted from the grid meter #2 as | ||
| # one COALESCE term, and consumption is clamped at zero. | ||
| assert graph.consumer_formula() == "MAX(#2 - COALESCE(#3, #4, 0.0), 0.0)" | ||
|
|
||
|
|
||
| def test_relay_is_passthrough() -> None: | ||
| """`Relay` maps to cg's `Breaker`, a pass-through category. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 New
InvalidGraphErrormessage format is documented as breaking but untestedThis release note flags a user-facing breaking change: graph validation failures now raise
InvalidGraphErrorwith a message startingGraph validation failed:(one indented line per problem), replacing the oldInvalidGraph: <description>format, and explicitly warns that "Code that matches on the message text of validation failures must be updated."No test pins this new format. The only related test,
tests/test_microgrid_component_graph.py:466, asserts the exception type only (pytest.raises(InvalidGraphError)), not the message. A future crate bump could silently change the message again with nothing to catch it.Impact
A behavior the PR itself calls out as breaking for downstream code has no regression guard. Since the message is now a documented contract, drift would go unnoticed until it breaks a consumer.
Related locations
Suggested tests
Build a graph that fails validation (rather than fails at graph construction) and assert the message starts with
Graph validation failed:, so the documented format is pinned.🔵 Low Severity: nice-to-have coverage for a documented breaking behavior.