Skip to content

Conversation

@pesap
Copy link
Collaborator

@pesap pesap commented Sep 9, 2025

Plexos test is broken and will be migrated to r2x-plexos eventually.

@pesap pesap requested review from Copilot and jarry7 September 9, 2025 16:43
Copy link

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 adds new hydro power structs (HydroReservoir and HydroTurbine) for PSY 5 compatibility and implements backward compatibility through an upgrader that converts legacy HydroEnergyReservoir components.

Key changes:

  • New hydro component models (HydroReservoir, HydroTurbine) and cost structure (HydroReservoirCost)
  • PSY4 to PSY5 upgrader that converts HydroEnergyReservoir into separate reservoir and turbine components
  • Comprehensive test coverage for new models and upgrade functionality

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/r2x/models/generators.py Adds new HydroReservoir and HydroTurbine model classes
src/r2x/models/costs.py Adds HydroReservoirCost class and example method for HydroGenerationCost
src/r2x/enums.py Adds ReservoirDataType enum for reservoir data types
src/r2x/upgrader/psy4_to_psy5.py Implements upgrader logic to convert legacy components
tests/upgrader/test_psy4_to_psy5.py Tests for the PSY4 to PSY5 upgrader functionality
tests/test_hydro_reservoir.py Tests for new hydro component models
src/r2x/models/__init__.py Updates imports to include new components

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

"intake_elevation": ext.get("intake_elevation", 0.0),
"head_to_volume_factor": ext.get("head_to_volume_factor", {"points": []}),
"operation_cost": HydroReservoirCost().model_dump(round_trip=True),
"level_data_type": str(ReservoirDataType.USABLE_VOLUME), # NOTE: Is this a good default?
Copy link

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

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

[nitpick] The inline comment raises a question about the default value but provides no guidance. Consider either removing the comment if this is the intended default, or documenting why this specific default was chosen.

Suggested change
"level_data_type": str(ReservoirDataType.USABLE_VOLUME), # NOTE: Is this a good default?
"level_data_type": str(ReservoirDataType.USABLE_VOLUME), # Default to USABLE_VOLUME to match typical reservoir usage in PSY5

Copilot uses AI. Check for mistakes.
return HydroGenerationCost(
fixed=0.0,
variable=CostCurve(
value_curve=LinearCurve(10), power_units=UnitSystem.NATURAL_UNITS, vom_cost=LinearCurve(5.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why non-zero by default?

),
)
efficiency: float = 1.0
conversion_factor: float = 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why default to 1.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mimic psy5. assumptions

)
active_power_pump: float = 0.0
efficiency: TurbinePump = TurbinePump(pump=1.0, turbine=1.0)
transition_time: TurbinePump = TurbinePump(pump=0.0, turbine=0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why default to 1.0?

jarry7
jarry7 previously requested changes Sep 9, 2025
Copy link
Contributor

@jarry7 jarry7 left a comment

Choose a reason for hiding this comment

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

Small comments provided in-line.

pesap and others added 3 commits September 9, 2025 14:49
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: pesap <pesap@users.noreply.github.com>
@jarry7 jarry7 dismissed their stale review September 9, 2025 21:42

Addressed by @pesap

@pesap pesap merged commit 8a26bb5 into v2.0.0 Sep 9, 2025
@pesap pesap deleted the ps/hydro branch September 9, 2025 21:43
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.

3 participants