Skip to content
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

Get rid of costs to virtual charging & discharging in storage_all.jl #608

Merged
merged 17 commits into from
Feb 11, 2024

Conversation

sambuddhac
Copy link
Collaborator

@sambuddhac sambuddhac commented Jan 8, 2024

Description

This PR minimizes the stray effect of virtual charging and discharging costs in storage_all.jl (Since virtual operations are theoretical and often lead to absurd outcomes).

What type of PR is this? (check all applicable)

  • Bug Fix
  • Performance Improvements

Related Tickets & Documents

Issue numbers fixed: Fixes Issue #604

Checklist

  • Code changes are sufficiently documented; i.e. new functions contain docstrings and .md files under /docs/src have been updated if necessary.
  • The latest changes on the target branch have been incorporated, so that any conflicts are taken care of before merging. This can be accomplished either by merging in the target branch (e.g. 'git merge develop') or by rebasing on top of the target branch (e.g. 'git rebase develop'). Please do not hesitate to reach out to the GenX development team if you need help with this.
  • Code has been tested to ensure all functionality works as intended.
  • CHANGELOG.md has been updated (if this is a 'notable' change).
  • I consent to the release of this PR's code under the GNU General Public license.

How this can be tested

Updated the test settings files by incorporating new settings parameter and modified objective function value for successful tests

Post-approval checklist for GenX core developers

After the PR is approved

  • Check that the latest changes on the target branch are incorporated, either via merge or rebase
  • Remember to squash and merge if incorporating into develop

@cfe316
Copy link
Collaborator

cfe316 commented Jan 8, 2024

Thanks, Sam.

It looks like VRE_STOR.jl lines 2036-2061 also has a virtual charging scheme.
This is mentioned in the comments/documentation there as well.

Please check that (the addition of virtual charge/discharge VOM costs to the Objective) is removed from any documentation as well.

@sambuddhac
Copy link
Collaborator Author

Thanks Jacob !!! Good catch. I will make sure that.

@sambuddhac sambuddhac requested a review from cfe316 January 13, 2024 00:19
@sambuddhac
Copy link
Collaborator Author

@cfe316 @gmantegna @wilson-ricks @lbonaldo I need to modify the doc pages and also the automated test script a little. On to it. Would be helpful is any/some/all of you can review the code in the mean-time and see if I am doing the right things here. Thanks bunches !!!! 😃

@@ -2034,28 +2035,28 @@ function vre_stor_capres!(EP::Model, inputs::Dict, setup::Dict)

#Variable costs of DC "virtual charging" for technologies "y" during hour "t" in zone "z"
@expression(EP, eCVar_Charge_DC_virtual[y in DC_CHARGE,t=1:T],
inputs["omega"][t]*by_rid(y,:Var_OM_Cost_per_MWh_Charge_DC)*vCAPRES_DC_CHARGE[y,t]/by_rid(y,:EtaInverter))
inputs["omega"][t]*(setup["VirtualChargeDischargeCost"]./scale_factor)*vCAPRES_DC_CHARGE[y,t]/by_rid(y,:EtaInverter))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could define a new variable and then use it in all the expressions below (in this way, you would divide only once):

virtual_chargedischarge_cost = setup["VirtualChargeDischargeCost"] ./ scale_factor
@expression(EP, eCVar_Charge_DC_virtual[y in DC_CHARGE,t=1:T],
        inputs["omega"][t]*(virtual_chargedischarge_cost * vCAPRES_DC_CHARGE[y,t]/by_rid(y,:EtaInverter))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done !!!

Copy link
Collaborator

@lbonaldo lbonaldo left a comment

Choose a reason for hiding this comment

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

Thanks @sambuddhac for this PR! I just added a simple comment.

@sambuddhac
Copy link
Collaborator Author

Thanks @sambuddhac for this PR! I just added a simple comment.

Don't see your latest comment. Could you please write it here once again? Thanks !!!

CHANGELOG.md Outdated
@@ -38,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Fix access of eELOSSByZone expr before initialization (#541)
- Correctly write unmet reserves (in reg_dn.csv) (#575)
- Correctly scale total reserves column (in reg_dn.csv) (#594)
- Fixes issue #604
Copy link
Collaborator

@cfe316 cfe316 Feb 5, 2024

Choose a reason for hiding this comment

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

It's useful to summarize what issue is being fixed here in the CHANGELOG, so that a user can understand whether it applied to their cases.

@@ -9,6 +9,7 @@ function default_settings()
"CapacityReserveMargin" => 0,
"CO2Cap" => 0,
"StorageLosses" => 1,
"VirtualChargeDischargeCost" => 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be useful to note here in a comment in that unlike the other the settings which are basically binary values, this thing has units of $/MWh.

@@ -32,6 +33,8 @@ function storage_all!(EP::Model, inputs::Dict, setup::Dict)
# Energy withdrawn from grid by resource "y" at hour "t" [MWh] on zone "z"
@variable(EP, vCHARGE[y in STOR_ALL, t=1:T] >= 0);

virtual_discharge_cost=setup["VirtualChargeDischargeCost"]./scale_factor
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's unclear why ./ is used since both the parameter and scale_factor are scalars.

@cfe316
Copy link
Collaborator

cfe316 commented Feb 5, 2024

Hi Sam, I've added a few comments about the code and about the changelog. Thanks.

  • I think also there needs to be a description of the new Settings parameter added to the documentation.

@@ -10,6 +10,7 @@ function storage_all!(EP::Model, inputs::Dict, setup::Dict)
dfGen = inputs["dfGen"]
Reserves = setup["Reserves"]
CapacityReserveMargin = setup["CapacityReserveMargin"]
scale_factor = setup["ParameterScale"] == 1 ? ModelScalingFactor : 1
Copy link
Collaborator

@cfe316 cfe316 Feb 5, 2024

Choose a reason for hiding this comment

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

It might be nice to think about loading this value in some other function, perhaps during load-time. In general, ModelScalingFactor and setup["ParameterScale"] are only referenced during the load_* and write_* phases, not during model construction. (The sole exception is in the hydrogen electrolyzer code.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps this could be added to the inputs[] dictionary? @lbonaldo . Or added to each relevant resource, once that's done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think that would simplify the usage of the scaling factor. Something like:

inputs["scale_factor"] = setup["ParameterScale"] == 1 ? ModelScalingFactor : 1

in load_inputs, and then inside the functions we get the value from the inputs Dict?

scale_factor = inputs["scale_factor"]

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good idea! I was actually thinking of constructing the VirtualChargeDischargeCost at load time and sticking that in inputs!

Since virtual operations are theoretical and often lead to absurd outcomes.
With a new settings parameters, "VirtualChargeDischargeCost."
Update storage_all.jl with "VirtualChargeDischargeCost setting parameter to account for cost of virtual charging and discharging
With VOM replaced by setup["VirtualChargeDischargeCost"]
Added VirtualChargeDischargeCost settings parameter
Fix minor typos
Added consideration of model scaling to the virtual charge and discharge cost
Updated virtual charging and discharging with model scaling factors
Changed the value of the virtual charge discharge cost settings parameter to 1
Changed the value of the virtual charge discharge cost settings parameter to 1
sambuddhac and others added 4 commits February 11, 2024 18:17
with virtual_discharge_cost=setup["VirtualChargeDischargeCost"]./scale_factor
With expression virtual_discharge_cost=setup["VirtualChargeDischargeCost"]./scale_factor
@lbonaldo lbonaldo self-requested a review February 11, 2024 23:40
Copy link
Collaborator

@lbonaldo lbonaldo left a comment

Choose a reason for hiding this comment

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

This now looks good to me.

@lbonaldo lbonaldo merged commit b2b3987 into develop Feb 11, 2024
9 checks passed
@lbonaldo lbonaldo deleted the sambuddhac-patch-1 branch February 11, 2024 23:47
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.

Variable costs should not be assigned to virtual storage charge/discharge
3 participants