Baseclass for Storage Performance Models by elenya-grant · Pull Request #624 · NatLabRockies/H2Integrate

elenya-grant · 2026-03-25T18:48:01Z

Baseclass for Storage Performance Models

This PR introduces a storage performance baseclass. The baseclass will help standardize the inputs and outputs of storage performance models.

The storage baseclass is in h2integrate/storage/storage_baseclass.py. This would be inherited by the following storage performance models:

StoragePerformanceModel in h2integrate/storage/storage_performance_model.py
PySAMBatteryPerformanceModel in h2integrate/storage/battery/pysam_battery.py
StorageAutoSizingModel in h2integrate/storage/simple_storage_auto_sizing.py

Some notes on the current performance baseclass:

the setup() method includes logic that would make it easy-to-use for all storage performance models, that is why it checks the configuration class for certain keys.
the run_storage() method is the main method that should be called at the end of compute(). This takes in the necessary information to simulate the storage performance and set the outputs
the simulate() method currently exists in the baseclass. This method would be over-written in the PySAMBatteryPerformanceModel

This PR is ready for a full-review.

Section 1: Type of Contribution

Section 2: Draft PR Checklist

Open draft PR
Describe the feature that will be added
Fill out TODO list steps
Describe requested feedback from reviewers on draft PR
[-] Complete Section 7: New Model Checklist (if applicable)

TODO (once reviewers provided initial feedback):

Update storage performance models and configuration classes to inherit the baseclass and base configuration class
Remove commented out code from the baseclass file

Type of Reviewer Feedback Requested (on Draft PR)

Structural feedback:

Implementation feedback:

I like the setup() method as-is, but I was curious what folks think about inputs that are added based on what's in the config? (Like charge rate and storage capacity only being inputs if those corresponding keys are in the config)
Options for the simulate() method (which is preferred?)
1. simulate() lives in the baseclass and is over-written by the PySAM battery model (as it is currently) OR
2. simulate() raises a not-implemented error in the baseclass and each model has their own version of it (meaning that the method would be the same in the StoragePerformanceModel and StorageAutoSizingModel

Other feedback:

Section 3: General PR Checklist

PR description thoroughly describes the new feature, bug fix, etc.
[-] Added tests for new functionality or bug fixes
Tests pass (If not, and this is expected, please elaborate in the Section 6: Test Results)
Documentation
- Docstrings are up-to-date
- Related docs/ files are up-to-date, or added when necessary
- Documentation has been rebuilt successfully
- [-] Examples have been updated (if applicable)
CHANGELOG.md
- At least one complete sentence has been provided to describe the changes made in this PR
- After the above, a hyperlink has been provided to the PR using the following format:
  "A complete thought. [PR XYZ]((https://github.com/NatLabRockies/H2Integrate/pull/XYZ)", where
  XYZ should be replaced with the actual number.

Section 3: Related Issues

Part of Issue #564

Section 4: Impacted Areas of the Software

Section 4.1: New Files

h2integrate/storage/storage_baseclass.py
- StoragePerformanceBaseConfig: Base configuration class for storage performance models, currently includes the attributes that are shared across all storage performance models.
- StoragePerformanceBase: new baseclass for storage performance models
  - setup(): initializes inputs and outputs of storage performance models
  - run_storage(): method that runs the simulate() method and calculates the outputs of the storage performance model
  - simulate(): simple storage simulate method that's used by StoragePerformanceModel and StorageAutoSizingModel (the PySAM battery model overwrites this method)

Section 4.2: Modified Files

h2integrate/storage/battery/pysam_battery.py: updated to use base config and baseclass
- with the inheritance of the new baseclass, some output naming changed. Specifically:
  - battery_electricity_discharge -> storage_electricity_discharge
  - battery_electricity_charge -> storage_electricity_charge
  - battery_electricity_out -> storage_electricity_out
h2integrate/storage/simple_storage_auto_sizing.py: updated to use base config and baseclass
h2integrate/storage/storage_performance_model.py: updated to use base config and baseclass

Section 4.3: Removed Files

h2integrate/storage/battery/battery_baseclass.py

Section 5: Additional Supporting Information

Section 6: Test Results, if applicable

…torage_model_baseclass

kbrunik · 2026-03-25T21:29:42Z

h2integrate/storage/storage_baseclass.py

+        # StoragePerformanceModel
+        # Do whatever pre-calculations are necessary, then run storage
+        self.dt_hr = int(self.options["plant_config"]["plant"]["simulation"]["dt"]) / (
+            60**2


switching it up from the 3600 I see

kbrunik · 2026-03-25T21:30:13Z

h2integrate/storage/storage_baseclass.py

+        )
+
+        # Input storage design parameters
+        if "max_charge_rate" in self.config.as_dict():


I like the if statements in the setup()

kbrunik · 2026-03-25T21:32:19Z

h2integrate/storage/storage_baseclass.py

+        # create inputs for pyomo control model
+        if "tech_to_dispatch_connections" in self.options["plant_config"]:
+            # get technology group name
+            # TODO: The split below seems brittle


Are we worried about this TODO now that it's coming into a base class?

I'm not sure. I can't think of a better way to do it. Any changes to this logic may also require updating the logic in the setup() method of PyomoControllerBaseClass. I personally don't know of a better way to handle this logic at the moment

@johnjasa or @RHammond2 any thoughts?

Perhaps pulling a specific entry from the split() output would be better. However, I think this should generally work as long as no two technologies have the same name, even if they are the same type (e.g. two batteries in the system should be battery_1 and battery_2 or something similar.

I feel like @johnjasa's framing of "should be ok" spells trouble down the road. I tend to avoid this kind of maneuver unless there's a validation involved. In a separate project, I entirely removed a large series of these combined naming structures as it created some extreme brittleness in naming conventions. The other concern is it becomes less clear over time what information is actually being captured, leading to difficulty in maintainability.

If someone has a suggestion on an alternative approach then I'm happy to try it.

Potentially we could give each tech it's own unique name, which I think could be helpful on multiple fronts not just dispatch.

self.tech_name = self.options.get("tech_name")

Then:

using_feedback_control = False connections = self.options["plant_config"].get("tech_to_dispatch_connections", []) for _source_tech, intended_dispatch_tech in connections: if intended_dispatch_tech == self.tech_name: self.add_discrete_input("pyomo_dispatch_solver", val=dummy_function) using_feedback_control = True break

I think this would be a bit more work than this PR is meant to do and relates very much so to #620. I think we could leave this for another PR.

kbrunik · 2026-03-25T21:35:16Z

h2integrate/storage/storage_baseclass.py

+            )
+        return outputs
+
+    def simulate(


I like the simulate method in the base class because then it makes it clear that it's a storage method. And I don't mind that it's overwritten in the PySAM battery model

Ditto, I'm glad to see this in the base class

kbrunik · 2026-03-25T21:36:42Z

h2integrate/storage/storage_baseclass.py

+            charge_rate, discharge_rate, storage_capacity, inputs, outputs, discrete_inputs
+        )
+
+    def run_storage(


When this is ready I would love to see a more robust docstring explaining how to use run_storage() in classes that inherit this base class

I just pushed up a doctoring to for the run_storage() method, but I will also add comments in the compute() method once we update the other storage performance models to inherit it.

Thank you for including an example in the docstring! Very helpful.

…rage_model_baseclass

jaredthomas68

I generally like where this is going. I have a lot of thoughts about the inputs and outputs, some of which I have shared in #521 and will likely need to be addressed in later PRs. Good job bringing more order and clarity to the dispatch/control code.

jaredthomas68 · 2026-03-26T19:24:03Z

h2integrate/storage/storage_baseclass.py

+        # create inputs for pyomo control model
+        if "tech_to_dispatch_connections" in self.options["plant_config"]:
+            # get technology group name
+            # TODO: The split below seems brittle


Perhaps pulling a specific entry from the split() output would be better. However, I think this should generally work as long as no two technologies have the same name, even if they are the same type (e.g. two batteries in the system should be battery_1 and battery_2 or something similar.

jaredthomas68 · 2026-03-26T21:58:15Z

h2integrate/storage/storage_baseclass.py

+            )
+        return outputs
+
+    def simulate(


Ditto, I'm glad to see this in the base class

…rage_model_baseclass

kbrunik

Thanks @elenya-grant for putting this together! It really helps clarify what's unique to each storage method and the base class is very clearly laid out with nice docstrings 😎

I had a couple of small comments regarding the docstrings but should be pretty easy to resolve and won't require another re-review. Thanks again!

kbrunik · 2026-03-29T11:51:29Z

h2integrate/storage/battery/pysam_battery.py

Love how much simpler the pysam_battery is now!!

kbrunik · 2026-03-29T11:56:36Z

h2integrate/storage/storage_baseclass.py

+            demand, and unused commodities.
+        simulate(commodity_in, commodity_demand, time_step_duration, control_variable,
+            sim_start_index=0):
+            Simulates the storage behavior across timesteps using input commodity as control.


I'm not totally sure what it means when you say "using input commodity as control", could this be revised?

kbrunik · 2026-03-29T11:57:28Z

h2integrate/storage/storage_baseclass.py

+
+
+    Notes:
+        - Default timestep is 1 hour (``dt=1.0``).


kbrunik · 2026-03-29T12:14:44Z

h2integrate/storage/storage_baseclass.py

+        # create inputs for pyomo control model
+        if "tech_to_dispatch_connections" in self.options["plant_config"]:
+            # get technology group name
+            # TODO: The split below seems brittle


Potentially we could give each tech it's own unique name, which I think could be helpful on multiple fronts not just dispatch.

self.tech_name = self.options.get("tech_name")

Then:

using_feedback_control = False connections = self.options["plant_config"].get("tech_to_dispatch_connections", []) for _source_tech, intended_dispatch_tech in connections: if intended_dispatch_tech == self.tech_name: self.add_discrete_input("pyomo_dispatch_solver", val=dummy_function) using_feedback_control = True break

I think this would be a bit more work than this PR is meant to do and relates very much so to #620. I think we could leave this for another PR.

kbrunik · 2026-03-29T12:18:21Z

h2integrate/storage/storage_baseclass.py

+            charge_rate, discharge_rate, storage_capacity, inputs, outputs, discrete_inputs
+        )
+
+    def run_storage(


Thank you for including an example in the docstring! Very helpful.

kbrunik · 2026-03-29T12:21:03Z

h2integrate/storage/storage_baseclass.py

+        compute(inputs, outputs, discrete_inputs, discrete_outputs):
+            Runs the storage model for a simulation timestep,
+            updating outputs such as SOC, charge/discharge limits, unmet
+            demand, and unused commodities.


Add run_storage() to the docstring?

elenya-grant added 3 commits March 23, 2026 11:36

added draft storage model baseclass

070285e

Merge branch 'develop' of github.com:NREL/H2Integrate into dispatch/s…

a8535f4

…torage_model_baseclass

updated draft storage baseclass

f94088c

elenya-grant added ready for review This PR is ready for input from folks dispatch related to dispatch and control labels Mar 25, 2026

elenya-grant requested review from genevievestarke, jaredthomas68, johnjasa and kbrunik March 25, 2026 18:48

kbrunik reviewed Mar 25, 2026

View reviewed changes

elenya-grant added 2 commits March 25, 2026 17:01

Merge remote-tracking branch 'h2i_upstream/develop' into dispatch/sto…

a48164e

…rage_model_baseclass

added docstring to run_storage method

c0a3b87

elenya-grant mentioned this pull request Mar 26, 2026

Reconsider storage performance model outputs #521

Open

minor updates to comments in storage_model_baseclass

8e7c514

jaredthomas68 reviewed Mar 26, 2026

View reviewed changes

elenya-grant added 6 commits March 27, 2026 09:35

Merge remote-tracking branch 'h2i_upstream/develop' into dispatch/sto…

7dbe7da

…rage_model_baseclass

updated storage performance model to inherit baseclass

3aaca71

fixed storage performance model and storage baseclass

27c1bdb

updated storage autosizing model

f7cebbc

updated pysam battery to use baseclass and removed battery_baseclass

17e241d

removed commented out code and simplified some docstrings

1029930

elenya-grant marked this pull request as ready for review March 27, 2026 16:26

elenya-grant requested review from jaredthomas68 and kbrunik March 27, 2026 16:26

kbrunik approved these changes Mar 29, 2026

View reviewed changes

removed unnecessary docstrings

78b3000

updated changelog

fce6042

Conversation

elenya-grant commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Baseclass for Storage Performance Models

Section 1: Type of Contribution

Section 2: Draft PR Checklist

TODO (once reviewers provided initial feedback):

Type of Reviewer Feedback Requested (on Draft PR)

Section 3: General PR Checklist

Section 3: Related Issues

Section 4: Impacted Areas of the Software

Section 4.1: New Files

Section 4.2: Modified Files

Section 4.3: Removed Files

Section 5: Additional Supporting Information

Section 6: Test Results, if applicable

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbrunik Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbrunik Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elenya-grant Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredthomas68 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbrunik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

elenya-grant commented Mar 25, 2026 •

edited

Loading

kbrunik Mar 25, 2026 •

edited

Loading

kbrunik Mar 25, 2026 •

edited

Loading

elenya-grant Mar 25, 2026 •

edited

Loading