Skip to content

Conversation

@ktehranchi
Copy link
Collaborator

Changes proposed in this Pull Request

  • Refactors the Energy Reserve Margin (ERM) opt to vectorize constraint creation, adds test case for multi-region reserves
  • Unit-commitment Parameters for thermal plants were incorrectly imputed by taking averages over the wrong fields. This lead to mixing of variable fuel cost based parameters from fixed start-up fuel consumption parameters.... tldr some OCGTs had way to large of start-up costs which are now fixed.
  • user @mailyorlik identified plants which were mis-assigned BA due to imprecise ReEDS shapes. I added some logic here that should fix this issue to ensure plants at the edges of interconnections (EROCT/EI) are not mis-aligned. But at the seams of balance authorities within an interconnection, this issue likely still persists and will need to be addressed.

Checklist

  • I tested my contribution locally and it seems to work fine.
  • Code and workflow changes are sufficiently documented.
  • Changed dependencies are added to envs/environment.yaml.
  • Changes in configuration options are added in all of config.default.yaml.
  • Changes in configuration options are also documented in doc/configtables/*.csv.

@ktehranchi ktehranchi requested a review from Copilot October 21, 2025 20:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the Energy Reserve Margin (ERM) optimization to improve performance through vectorization, fixes unit-commitment parameter imputation errors that caused incorrect start-up costs for thermal plants, and addresses plant assignment issues at interconnection boundaries.

Key Changes:

  • Vectorized ERM constraint creation by replacing nested loop-based constraint creation with PyPSA's nodal balance constraint framework
  • Fixed unit-commitment parameter imputation by separating fixed and fuel-based startup costs and adding build decade to aggregation fields
  • Enhanced plant-to-BA assignment logic to handle interconnection boundary cases using ReEDS membership data

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
workflow/scripts/test/test_reserves.py Adds comprehensive test for non-overlapping multi-region ERM constraints with proper verification
workflow/scripts/test/fixtures/non_overlapping_erm.csv Test fixture defining two non-overlapping regions with different reserve margins
workflow/scripts/solve_network.py Removes deprecated PRM constraint function and fixes clip_p_max_pu assignment bug
workflow/scripts/plot_statistics.py Adds global_constraints output to statistics
workflow/scripts/opts/reserves.py Major refactor: vectorizes ERM constraints, removes PRM function, improves dual storage
workflow/scripts/opts/_helpers.py Adds support for region column in bus filtering
workflow/scripts/constants.py Moves EIA mapping data from build_powerplants to shared constants
workflow/scripts/build_powerplants.py Fixes startup cost calculation and improves parameter imputation with build decade
workflow/scripts/add_electricity.py Implements interconnection boundary logic for plant assignment
workflow/rules/postprocess.smk Adds global_constraints output path
workflow/rules/build_electricity.smk Adds ReEDS shapes and membership inputs
.pre-commit-config.yaml Pins black version for blackdoc compatibility

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +74 to +79
n.generators_t.p_min_pu = df.where(df > solve_opts["clip_p_max_pu"], other=0.0)

df = n.links_t.p_max_pu
n.links_t.p_max_pu = df.where(df > solve_opts["clip_p_max_pu"], other=0.0)
df = n.links_t.p_min_pu
n.links_t.p_min_pu = df.where(df > solve_opts["clip_p_max_pu"], other=0.0)
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clip threshold for p_min_pu should use a different comparison. Setting values below the threshold to 0.0 is incorrect for minimum power limits. Line 74 should either use a different threshold or the clipping logic should be df.where(df < solve_opts['clip_p_max_pu'], other=0.0) to properly handle minimum values.

Suggested change
n.generators_t.p_min_pu = df.where(df > solve_opts["clip_p_max_pu"], other=0.0)
df = n.links_t.p_max_pu
n.links_t.p_max_pu = df.where(df > solve_opts["clip_p_max_pu"], other=0.0)
df = n.links_t.p_min_pu
n.links_t.p_min_pu = df.where(df > solve_opts["clip_p_max_pu"], other=0.0)
n.generators_t.p_min_pu = df.where(df < solve_opts["clip_p_max_pu"], other=0.0)
df = n.links_t.p_max_pu
n.links_t.p_max_pu = df.where(df > solve_opts["clip_p_max_pu"], other=0.0)
df = n.links_t.p_min_pu
n.links_t.p_min_pu = df.where(df < solve_opts["clip_p_max_pu"], other=0.0)

Copilot uses AI. Check for mistakes.
)
# Create model variables to track storage contributions (only once)
c = "StorageUnit"
if not n.storage_units.empty and f"{c}-p_dispatch_RESERVES" not in model.variables:
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check for existing variables should include all three StorageUnit reserve variables, not just p_dispatch_RESERVES. If constraints fail midway through the three model.add_variables calls on lines 467-473, subsequent calls to this function could skip variable creation but still attempt to create constraints, leading to missing variable errors.

Suggested change
if not n.storage_units.empty and f"{c}-p_dispatch_RESERVES" not in model.variables:
reserve_vars = [f"{c}-p_dispatch_RESERVES", f"{c}-p_store_RESERVES", f"{c}-state_of_charge_RESERVES"]
if not n.storage_units.empty and not all(var in model.variables for var in reserve_vars):

Copilot uses AI. Check for mistakes.
Comment on lines +601 to +606
erm_col = global_constraint_columns[0]
erm_dual_df = (
erm_dual.to_dataframe()[erm_col].reset_index().set_index(["period", "timestep"]).pivot(columns="Bus")
)
erm_dual_df.columns = erm_dual_df.columns.get_level_values(1)
n.buses_t["erm_price"].update(erm_dual_df)
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic assumes only one ERM constraint column per dual result, but with multiple regions (as tested in test_multiple_non_overlapping_erms), there could be multiple columns ending with '_ERM'. Using only the first column [0] will lose data from other regions. The code should iterate over all matching columns or use a more specific column name pattern.

Suggested change
erm_col = global_constraint_columns[0]
erm_dual_df = (
erm_dual.to_dataframe()[erm_col].reset_index().set_index(["period", "timestep"]).pivot(columns="Bus")
)
erm_dual_df.columns = erm_dual_df.columns.get_level_values(1)
n.buses_t["erm_price"].update(erm_dual_df)
# Iterate over all ERM columns to handle multiple regions
for erm_col in global_constraint_columns:
erm_dual_df = (
erm_dual.to_dataframe()[erm_col].reset_index().set_index(["period", "timestep"]).pivot(columns="Bus")
)
erm_dual_df.columns = erm_dual_df.columns.get_level_values(1)
n.buses_t["erm_price"].update(erm_dual_df)

Copilot uses AI. Check for mistakes.
Comment on lines +495 to +496
plants["start_fuel_cost"] = plants.start_fuel_mmbtu * plants.fuel_cost
plants["start_up_cost"] = plants.startup_cost_fixed + plants.start_fuel_cost
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The startup cost calculation should occur before imputation (before line 491), not after. Currently, plants with NaN fuel_cost after imputation will have NaN start_fuel_cost, negating the benefit of the imputation fixes described in the PR.

Copilot uses AI. Check for mistakes.
plants_nearshore = plants_nearshore.to_crs(epsg=4326)
plants_filt = pd.concat([plants_filt, plants_nearshore])
plants_no_region_all_shapes = plants_no_region_all_shapes.to_crs(epsg=4326)
reeds_memberships.loc[reeds_memberships.interconnect == "ercot", "interconnect"] = "texas"
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This in-place modification of the reeds_memberships DataFrame could cause unexpected behavior if the function is called multiple times or if the DataFrame is used elsewhere. Consider creating a copy: reeds_memberships = reeds_memberships.copy() before the modification, or use a non-mutating approach: reeds_memberships['interconnect'].replace({'ercot': 'texas'}).

Copilot uses AI. Check for mistakes.
Comment on lines +467 to 476
plants["p_min_pu"] = (
(plants.minimum_load_mw / plants.p_nom)
.clip(
upper=np.minimum(plants.summer_derate, plants.winter_derate),
lower=0,
)
.astype(float)
.fillna(0)
.mul(0.95)
)
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The hardcoded 0.95 multiplier on line 475 lacks explanation. This appears to be reducing the minimum load by 5%, but the reasoning should be documented in a comment or extracted to a named constant to improve code clarity.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant