-
Notifications
You must be signed in to change notification settings - Fork 67
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
Market bid cost with pwl cost curve #1182
base: market_bid_cost
Are you sure you want to change the base?
Market bid cost with pwl cost curve #1182
Conversation
purboday
commented
Dec 9, 2024
- In src/devices_models/devices/common/objective_function/market_bid.jl added function _add_vom_cost_to_objective!.
- Updated "Test Thermal Generation MarketBidCost models" in test/test_device_thermal_generation_constructors.jl to make fixed_market_bid_cost run and pass.
- Other diffs caused by the formatter.
test/run_market_bid_cost.jl
Outdated
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.
This file is useful but it shouldn't be checked out into the repo. I'd like the original test to be fixed.
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.
Deleted the file.
…com/purboday/PowerSimulations.jl into market-bid-cost-with-pwl-cost-curve
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1182 +/- ##
==========================================
+ Coverage 77.23% 77.67% +0.43%
==========================================
Files 121 121
Lines 13548 13641 +93
==========================================
+ Hits 10464 10595 +131
+ Misses 3084 3046 -38
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
A few unsolicited comments from the software development point of view
@@ -179,6 +179,36 @@ function _get_pwl_cost_expression( | |||
) | |||
end | |||
|
|||
function _get_pwl_cost_expression_decremental(container::OptimizationContainer, |
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.
Can this be implemented in a way that does not duplicate as much code from the incremental version? Maybe refactor both functions to call a helper function with the common code?
@@ -320,6 +364,43 @@ function _add_pwl_term!( | |||
return pwl_cost_expressions | |||
end | |||
|
|||
function _add_pwl_term_decremental!(container::OptimizationContainer, |
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.
Same note about code duplication here
""" | ||
Check if deceremental pwl offer curve is monotonically decreasing. | ||
""" | ||
function _is_convex_decremental(pwl:: PSY.PiecewiseStepData) |
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.
Ideally this function would go in the same place as the existing is_convex
, in IS/PSY. Maybe is_concave
?
end | ||
|
||
|
||
function _add_vom_cost_to_objective!(container::OptimizationContainer, |
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.
What is the difference between these two _add_vom_cost_to_objective!
methods?
|
||
|
||
"""Overload get_variable for MarketBidCost. Returns nothing""" | ||
PSY.get_variable(value::PSY.MarketBidCost) = nothing |
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.
@GabrielKS and @purboday I changed the target of this PR to a new branch to fully develop and test this feature including the simulation capability |