Make sure `storage_baseclass` handles arbitrary `dt` by vijay092 · Pull Request #753 · NatLabRockies/H2Integrate

vijay092 · 2026-05-15T23:51:54Z

Make sure `storage_baseclass` handles arbitrary `dt`

storage_baseclass contains several unit inconsistencies when dt is non-hourly. This PR addresses them. This was brought up in #738.

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:

Step 1
Step 2

Type of Reviewer Feedback Requested (on Draft PR)

Structural feedback:

Implementation feedback:

Other feedback:

Section 3: General PR Checklist

Section 4: Related Issues

Section 5: Impacted Areas of the Software

Section 5.1: New Files

path/to/file.extension
- method1: What and why something was changed in one sentence or less.

Section 5.2: Modified Files

h2integrate/storage/test/test_storage_performance_model.py
h2integrate/storage/storage_baseclass.py

Section 6: Additional Supporting Information

Section 7: Test Results, if applicable

Section 8 (Optional): New Model Checklist

johnjasa

Thanks for this, Sanjana! I have one suggested change where I want your thoughts and input.

johnjasa · 2026-05-19T16:55:47Z

    _time_step_bounds = (
        3600,
        3600,
    )  # (min, max) time step lengths (in seconds) compatible with this model


Could you please update this so it correctly reflects a less restrictive bound on the timestep? Are there any other limitations not addressed in this PR that would affect how valid different timesteps are?

Good point. I'll expand the StoragePerformanceBase._time_step_bounds to reflect that the math now handles any dt. The subclasses (StoragePerformanceModel) still have (3600, 3600). PySAMBatteryPerformanceModel also only supports hourly. @jaredthomas68 @elenya-grant could you also weigh in here?

Here's the bounds I think each storage model should have:

StorageAutoSizingModel: (3600, 3600) - should not be changed until its tested

StoragePerformanceModel: (1, 3600)

PySAMBatteryPerformanceModel: (3600, 3600) - should not be changed until its tested

StoragePerformanceBase: this could have (3600, 3600) or (1, 3600) since it's overwritten by the subclasses.

For StoragePerformanceBase, I changed the bounds to (1,36000) to add the test Elenya suggested (dt > 1hr).

johnjasa · 2026-05-19T16:56:03Z

+def test_storage_half_hourly_known_outputs(subtests):
+    """Verify SOC, charge/discharge profiles, and scalar outputs against calculated
+    values for a simple scenario at dt=1800s (30-min dt).
+
+    Scenario (4 timesteps * 30 min = 2 hours total):
+        t0, t1: charge at 10 kg/h - stores 5 kg each step
+        t2, t3: discharge at 10 kg/h — removes 5 kg each step
+
+    With capacity=40 kg, init_soc=0.1, eff=1.0, min_soc=0.1, max_soc=1.0:
+        SOC[0] = 0.1 + 5/40 = 0.225
+        SOC[1] = 0.225 + 5/40 = 0.35
+        SOC[2] = 0.35  - 5/40 = 0.225
+        SOC[3] = 0.225 - 5/40 = 0.1
+    total_hydrogen_produced = (-10 - 10 + 10 + 10) * 0.5 hr = 0 kg
+    standard_capacity_factor = (10+10)*0.5 / (10 * 4 * 0.5) = 10/20 = 0.5 -> 50 %
+    """


Fantastic and clear test! Thank you for this.

elenya-grant

I left a few small suggestions but overall this looks good to me! Your approach is very clear and the math makes sense to me! Thanks for adding this feature!

I think some nice-to-haves (but not necessary) would be:

test using kg/s as the commodity rate units but using a timestep of 30 min
test using kW and kW*h
perhaps a test for dt>3600 (only if the upper limit on _time_step_bounds is changed from 3600)

elenya-grant · 2026-05-19T20:44:32Z

+                headroom = (soc_max - soc) * storage_capacity / self.dt_amount

                # charge available based on the available input commodity
                charge_available = commodity_available[sim_start_index + t]


Just checking here - we expect that the headroom, charge_available, the input storage_dispatch_commands, etc are in commodity_rate_units?

elenya-grant · 2026-05-19T20:50:31Z

+@pytest.mark.regression
+def test_storage_half_hourly_known_outputs(subtests):


Suggested change

@pytest.mark.regression

def test_storage_half_hourly_known_outputs(subtests):

@pytest.mark.regression

@pytest.mark.parametrize("n_timesteps", [4])

def test_storage_half_hourly_known_outputs(subtests, plant_config_non_hourly):

elenya-grant · 2026-05-19T20:51:26Z

+    total_hydrogen_produced = (-10 - 10 + 10 + 10) * 0.5 hr = 0 kg
+    standard_capacity_factor = (10+10)*0.5 / (10 * 4 * 0.5) = 10/20 = 0.5 -> 50 %
+    """
+    plant_config = {


If you take the suggestion I had above, you won't need to re-define the plant config here. You could just use plant_config_non_hourly instead.

elenya-grant · 2026-05-20T16:35:22Z

    _time_step_bounds = (
        3600,
        3600,
    )  # (min, max) time step lengths (in seconds) compatible with this model


Here's the bounds I think each storage model should have:

StorageAutoSizingModel: (3600, 3600) - should not be changed until its tested

StoragePerformanceModel: (1, 3600)

PySAMBatteryPerformanceModel: (3600, 3600) - should not be changed until its tested

StoragePerformanceBase: this could have (3600, 3600) or (1, 3600) since it's overwritten by the subclasses.

vijay092 · 2026-05-21T16:19:58Z

I left a few small suggestions but overall this looks good to me! Your approach is very clear and the math makes sense to me! Thanks for adding this feature!

I think some nice-to-haves (but not necessary) would be:

test using kg/s as the commodity rate units but using a timestep of 30 min

test using kW and kW*h

perhaps a test for dt>3600 (only if the upper limit on _time_step_bounds is changed from 3600)

Added additional tests!

johnjasa · 2026-05-21T22:20:14Z

+        self.dt_amount = om_units.convert_units(
+            self.dt,
+            "s",
+            f"({self.commodity_amount_units})/({self.commodity_rate_units})",
+        )


Great use of the built-in units conversion here!

johnjasa

Thanks for these changes and improvements, Sanjana! I think this is a great step towards a much more flexible H2I codebase. The storage models have quite a few intricacies, so I appreciate you digging into the models and improving them.

vijay092 added 2 commits May 15, 2026 17:33

Make sure storage_baseclass handles arbit dt

f5cc2ea

Format storage_baseclass

521de04

johnjasa requested a review from elenya-grant May 19, 2026 14:48

johnjasa and others added 4 commits May 19, 2026 09:48

Merge branch 'develop' into dt_hr

68a26ca

Add ability to handle different rate units

d74f80e

Add comments

58a56da

Run pre-commit

4d47919

johnjasa requested changes May 19, 2026

View reviewed changes

Expand _time_step_bounds

e0e40a6

elenya-grant approved these changes May 20, 2026

View reviewed changes

vijay092 added 2 commits May 21, 2026 10:16

Add tests for kgs and kwh

56052fe

Fix comments

42bc5e0

Merge branch 'develop' into dt_hr

1287374

johnjasa marked this pull request as ready for review May 21, 2026 19:42

vijay092 and others added 2 commits May 21, 2026 14:10

Merge branch 'develop' into dt_hr

8231ddd

Merge branch 'develop' into dt_hr

bd3d0ff

johnjasa reviewed May 21, 2026

View reviewed changes

johnjasa approved these changes May 21, 2026

View reviewed changes

johnjasa enabled auto-merge (squash) May 21, 2026 22:21

johnjasa merged commit b9f33f6 into NatLabRockies:develop May 21, 2026
12 checks passed

		@pytest.mark.regression
		def test_storage_half_hourly_known_outputs(subtests):

Conversation

vijay092 commented May 15, 2026

Make sure storage_baseclass handles arbitrary dt

Section 1: Type of Contribution

Section 2: Draft PR Checklist

TODO:

Type of Reviewer Feedback Requested (on Draft PR)

Section 3: General PR Checklist

Section 4: Related Issues

Section 5: Impacted Areas of the Software

Section 5.1: New Files

Section 5.2: Modified Files

Section 6: Additional Supporting Information

Section 7: Test Results, if applicable

Section 8 (Optional): New Model Checklist

Uh oh!

johnjasa 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!

elenya-grant 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!

vijay092 commented May 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnjasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Make sure `storage_baseclass` handles arbitrary `dt`