make battery charge positive and add demand component to plm opt exam…#745
make battery charge positive and add demand component to plm opt exam…#745jaredthomas68 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Thanks for adding this fix, Jared! I think it makes a lot of sense.
I think that we should be careful to change the charge output profile to be positive (a lot of tests are going to fail). I suppose my only reason to keep it negative is that I could imagine it being useful in some future use-case, and that having it negative makes it more clear that negative values mean charging and positive values mean discharging when looking at the storage_{commodity}_out, {commodity}_set_point or storage_dispatch_commands.
Responding to your requested feedback from reviewers: I think we should perhaps add a new output, such as "storage_{commodity}_charge_positive", instead. I'd be curious to hear what others think.
There was a problem hiding this comment.
storage_commodity_out is negative when charging and positive when discharging. The charge output profile is defined as always being negative - this change will cause a lot of tests to fail. I think we could instead add another output called storage_commodity_charge_positive or something.
There was a problem hiding this comment.
I have a feeling this will break things in Gen's tests as well.
There was a problem hiding this comment.
thanks for adding more subtests! This will definitely help with any future debugging of this example if it fails!
| timezone: -6 # MDT (UTC-6) | ||
| start_time: 2025/01/01 00:00:00 | ||
| technology_interconnections: | ||
| - [battery, electrical_load_demand, [storage_electricity_charge, electricity_demand]] |
There was a problem hiding this comment.
I like this solution but I still think we should think about adding a new storage output rather than changing the existing one.
There was a problem hiding this comment.
How is this different from - [battery, electrical_load_demand, [electricity_out, electricity_in]]
?
There was a problem hiding this comment.
electricity_out from storage is negative when charging and positive when discharging. Meaning that electricity_in to the demand component (which should be the electricity supplied to the demand component) would be positive and negative, which would increase the unmet_electricity_demand_out would be greater than the total demand when the battery is charging. So, actually I do think that [battery, electrical_load_demand, [electricity_out, electricity_in]] would be a feasible solution!
vijay092
left a comment
vijay092
left a comment
There was a problem hiding this comment.
Thanks, Jared! This all makes sense. Just flagging that we may want additional tests to confirm the wiring is working as expected.
| timezone: -6 # MDT (UTC-6) | ||
| start_time: 2025/01/01 00:00:00 | ||
| technology_interconnections: | ||
| - [battery, electrical_load_demand, [storage_electricity_charge, electricity_demand]] |
There was a problem hiding this comment.
How is this different from - [battery, electrical_load_demand, [electricity_out, electricity_in]]
?
| assert pytest.approx(total_energy_charged, rel=1e-3) == -2663.0 | ||
| assert pytest.approx(total_energy_charged, rel=1e-3) == 2663.0 | ||
|
|
||
| with subtests.test("Check energy balance with battery and grid"): |
There was a problem hiding this comment.
Do we need more tests to make sure the wiring and energy balance is correct? Maybe this could be a follow on PR but this is good for #664.
Examples I can think of: i) No grid purchase during discharge timesteps. ii) Grid purchase equals unmet demand
|
@jaredthomas68, thanks again for chatting about #664 today. Given that it's in develop now, where does that leave this PR? Are you planning other changes, should it be closed, or something else? |
I'm closing this PR for now, but I think we may want to come back to it later. |
4 participants
…ple 34
Add demand component to PLM opt and consider making storage charge positive
PR #664 is making it so charging can't happen without grid, so we need to make sure that the opt plm works with grid charging prior to that PR so the new changes in #664 can be tested against known results. I did some work on this and the solution I found was to make charge positive so that battery charge can be passed to the demand component.
I have not made all the sign changes that would be needed for this PR to be complete, but I would like to generate discussion around this to facilitate proper preparation for merging PR #664.
Section 1: Type of Contribution
Section 2: Draft PR Checklist
TODO:
Type of Reviewer Feedback Requested (on Draft PR)
Structural feedback:
Implementation feedback:
Other feedback:
Section 3: General PR Checklist
docs/files are up-to-date, or added when necessaryCHANGELOG.md"A complete thought. [PR XYZ]((https://github.com/NatLabRockies/H2Integrate/pull/XYZ)", where
XYZshould be replaced with the actual number.Section 4: Related Issues
Section 5: Impacted Areas of the Software
Section 5.1: New Files
path/to/file.extensionmethod1: What and why something was changed in one sentence or less.Section 5.2: Modified Files
path/to/file.extensionmethod1: What and why something was changed in one sentence or less.Section 6: Additional Supporting Information
Section 7: Test Results, if applicable
Section 8 (Optional): New Model Checklist
docs/developer_guide/coding_guidelines.mdattrsclass to define theConfigto load in attributes for the modelBaseConfigorCostModelBaseConfiginitialize()method,setup()method,compute()methodCostModelBaseClasssupported_models.pycreate_financial_modelinh2integrate_model.pytest_all_examples.pydocs/user_guide/model_overview.mddocs/section<model_name>.mdis added to the_toc.ymlgenerate_class_hierarchy.pyto update the class hierarchy diagram indocs/developer_guide/class_structure.md