Apply per-timeslice link limits as per-snapshot p_max_pu/p_min_pu series - #137
Open
nick-gorman wants to merge 12 commits into
Open
Apply per-timeslice link limits as per-snapshot p_max_pu/p_min_pu series#137nick-gorman wants to merge 12 commits into
nick-gorman wants to merge 12 commits into
Conversation
The new-format translator emits link limits per timeslice, in per-unit form, in a link_timeslice_limits table rather than as per-link series. Expanding them into per-snapshot p_max_pu / p_min_pu here at network build time, via the timeslice_snapshots mapping, keeps the pypsa-friendly directory small and reuses the same mapping that scopes the custom constraints. Each (link, attribute) series is seeded from the timeslice = NaN fallback row and the named timeslices are written over it. Seeding from the fallback rather than the links table's static value matters: since #126 the translator sets p_nom = max(forward, reverse) and ships p_max_pu=1.0 / p_min_pu=0.0 only as placeholders, so a series that fell back to them would over-permit forward flow and disable reverse flow at every snapshot no named timeslice covers. Dark until the orchestrator wiring lands: _add_links_to_network keeps its two-argument form for the current path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…ling for flag retirement "None when all link limits are static" read as a per-run choice, but on the new-format path the translator always emits limit rows and the links table's p_max_pu / p_min_pu are placeholders, so omitting the table there would silently mis-model every link. Spell out the two paths and mark the None handling with the existing FEATURE_FLAG_CLEANUP convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Args prose restated what the example can show directly: what each table looks like, and that the function returns None because the network is modified in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s its caller Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… them Filtering zero-p_nom links out of link_timeslice_limits made them the one class of link pypsa_build had to know about: their placeholder p_max_pu / p_min_pu did real work, and every consumer needed a caveat for links with no limit rows. Defining per-unit-of-zero as 0 at the point of division lets them flow through pypsa_build like any other link. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… gaps Seeding each series from the links table's placeholder p_max_pu / p_min_pu gave an uncovered snapshot a plausible value and hid the gap. The series are now built as one long table - every (link, attribute) pair at every snapshot, named-timeslice values merged in where active, the blank- timeslice fallback filled elsewhere - so a snapshot with neither is a NaN that raises with the affected links named, and the intermediate table can be asserted directly in tests. The merge form also replaces the per-pair label lookups and overwrite loop with plain pandas joins, so a named timeslice with no snapshots or a pair with no fallback row falls out of merge semantics rather than needing its own branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The to_string table made the message whitespace-sensitive; a plain comma-separated (investment_period, snapshot) list reads the same and lets the test pin the whole message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An all-blank timeslice column read from CSV comes back float64 and can't be merged onto the object-typed empty timeslice_snapshots. The proper fix is schema-typed reading of the pypsa-friendly tables (#138), so this records the case rather than casting at the merge site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…by extent The coverage check drew its universe from the limits table itself, so a (link, attribute) with no rows at all was never checked and the links table's placeholder p_max_pu / p_min_pu survived silently. The grid is now built from the non-extendable links x both attributes, so an existing link the limits never mention raises like a partially covered one does; expansion links keep their static values and are not checked. The error message now separates pairs undefined at every snapshot (typically no limit rows at all) from those undefined at only some, and samples uncovered snapshots only for the latter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-cell lookups can miss stray columns or a placeholder leaking through; a side-by-side frame with a non-placeholder p_min_pu pins that the links table's static values really are the limits when no limit tables are passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test described a blank-timeslice fallback of 0, but _new_parallel_path_rows emits explicit zeros per direction and named timeslice with no fallback. Now that zero rows reach pypsa_build's coverage check the two shapes behave differently, so the test exercises the real one and keeps the fallback form as a separate case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_link_attribute_snapshot_grid read as a noun, so the orchestrator line didn't say what the helper does. _create_* matches the verb-phrase convention the other helpers follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fNbcEWRpvRjndGUXvt4FV
EllieKallmier
approved these changes
Sep 2, 2026
EllieKallmier
left a comment
EllieKallmier
left a comment
Member
There was a problem hiding this comment.
nice, all makes sense and looks good to me :) comments are vibe-based and non blocking haha
| CQ-NQ_existing p_max_pu qld_peak_demand 0.857 # 1200/1400 | ||
| CQ-NQ_existing p_max_pu qld_winter_reference 1.0 # 1400/1400 | ||
| CQ-NQ_existing p_min_pu , -0.714 # fallback, -1000/1400 | ||
|
|
Member
There was a problem hiding this comment.
Could potentially benefit from having a zero-p_nom link in the I/O Example for full illustration?
| uncovered at every snapshot and those uncovered at only some. | ||
|
|
||
| I/O Example: | ||
| limits_per_snapshot: A p_max_pu at 2 snapshots (both NaN), |
Member
There was a problem hiding this comment.
I get this and am not against summarised I/O examples when it's this many layers deep + likely to be repetitive of caller's example, but in this case I think the ('name', 'attribute') structure of the returned tuples is a little bit lost. Just cause 'A p_max_pu at 2 snapshots' could just as easily read as a sentence as a 'name, attribute at 2 snapshots' description.
Suggested change
| limits_per_snapshot: A p_max_pu at 2 snapshots (both NaN), | |
| limits_per_snapshot (abbr.): | |
| name attribute ... value | |
| A p_max_pu ... # uncovered | |
| A p_min_pu ... # uncovered | |
| A p_min_pu ... 0.9 | |
| A p_min_pu ... # uncovered | |
| uncovered: DataFrame with tagged rows (above) |
(or something) to illustrate directly the return structure maybe? Not hugely important though tbh
| CQ-NQ_option_1, 0.9, -0.6 | ||
| """).set_index("Link") | ||
| # The existing link's static values are the untouched placeholders; its | ||
| # real limits live in links_t (covered by the tests above). |
Member
There was a problem hiding this comment.
Noting out loud for myself so I don't forget - PyPSA always chooses the time-varying (series) version of an attribute when both series and static values are defined for that attribute :)
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
2 participants
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.
Functionality for adding per timeslice transmission limits to the pypsa network in the pypsa_build module.
Pypsa friendly tables of the form (abreviated data):
Get transformed and inserted into the Pypsa model such that:
A few points:
timeslicecolumn is blank in every row comes back from CSV as float64 and can't be merged onto an emptytimeslice_snapshots. That's a general CSV-round-trip typing problem (Column dtypes change on CSV round-trip; apply schema types when reading pypsa-friendly tables #138) and the fix belongs at the read boundary; anxfailtest records it here.timesliceandtimeslice_idas column names, this PR works around it, but Rename the timeslices table's timeslice_id column to timeslice #140 will clean up to just usetimesliceeverywhere.