Framework to allow for variable dt by jaredthomas68 · Pull Request #653 · NatLabRockies/H2Integrate

jaredthomas68 · 2026-04-03T23:56:44Z

Framework to allow for variable dt

This PR contains changes to the framework to handle variable dt between different models so we can start integrating model changes one at a time to allow variable time steps.

I also adjusted the grid components to work with variable dt to demonstrate how the framework changes will work with actual models and test the integration.

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)

Ready for full review, I'm looking for any and all feedback.

Section 3: General PR Checklist

Section 3: Related Issues

This PR will resolve #648 and makes steps towards resolving #204.

The work on the grid components will also resolve #638.

Section 4: Impacted Areas of the Software

Section 4.1: New Files

Section 4.2: Modified Files

docs/developer_guide/adding_a_new_technology.md: variable time step info added
docs/intro.md: variable time step info added
docs/technology_models/grid.md: variable time step info added
h2integrate/converters/grid/grid.py: add 5 m to 1 h time step bounds
h2integrate/converters/grid/test/test_grid.py: unit and integration test for non-hour time step
h2integrate/core/h2integrate_model.py: check time step on a per-model basis
h2integrate/core/inputs/validation.py: make simulation length check account for variable dt and remove dt check
h2integrate/core/model_baseclasses.py: add self.dt to cost model base class
h2integrate/core/test/test_framework.py: add unit tests for time step check

Section 5: Additional Supporting Information

This PR is just the first step in a series of expected PRs to allow more flexibility in time step selection.

Section 6: Test Results, if applicable

Section 7 (Optional): New Model Checklist

elenya-grant

Thanks for introducing this PR! I had a couple of high-level suggestions/questions for other folks. I think this is the cleanest and least complicated way of introducing this flexibility - nice job!

Kinda big-picture consideration - should there be a valid time-step interval defined too? Like - for the grid, what if I tried to run a timestep of 563 seconds? Maybe thats fine but it seems weird. Should there be another parameter defined as like _time_step_interval? So that models can be run for timesteps:

valid_dts = np.arange(_time_step_min, _time_step_max, _time_step_interval)

(not asking for this to be implemented, but a question I think it worth considering)

I know this is still a draft, but I would like to see a test for the grid component using a smaller timestep.

elenya-grant · 2026-04-07T16:13:35Z

h2integrate/converters/grid/grid.py


+    _time_step_min = 300
+    _time_step_max = 3600
+


I like how you've added these attributes. I could see a similar thing perhaps being implemented in the future for simulation length. I'd be curious what your (and other people's thoughts) would be on whether these should be two separate variables (like you have) vs one tuple like this instead:

_time_step_bounds = (300, 3600)

Another question - why can't this have a min timestep of 1 sec? Why 300?

I think making the min 300 seconds for now makes a lot of sense, since that's the smallest resource step that is available and also the smallest grid price data resolution. I think that letting the time step be 1 second is not realistic for the steady state models we have in H2I. This is something I feel pretty strongly about, but what are others' opinions about this?

Thank you @elenya-grant. Also, I like the idea of using a tuple.

I agree with @genevievestarke on the minimum time step, at least for now. The 5-minute resource data floor, and the fact we are not modeling any transient behavior, is why I choose to limit the grid to 5-minute minimum time steps.

I think the only reason to not use timestep bounds would be so one or the other could be left out. Realistically, I think there will always be upper and lower limits, so I think it will be fine to use a tuple.

I switched to using a tuple

I wonder if in the more general implementation for the framework--like in h2integrate_model.py and the utilities files that house dt methods (not the already include converter/storage models)--we can allow dt to be less than 300 because I don't see a reason we couldn't theoretically allow users to put a signal of their choice in at a 1 second time resolution and use the other h2i capabilities such as the financial modeling with finer resolution performance data.

elenya-grant · 2026-04-07T16:19:55Z

h2integrate/core/h2integrate_model.py

+                )
+                raise ValueError(msg)
+
+        elif dt != 3600:


I think an alternative to this elif catch would be to add the _time_step_min and time_step_max to the CostModelBaseClass and PerformanceModelBaseClass and have them both default to 3600. I'm not saying this is better - but its an alternative approach.

I started doing this, but it seems like a lot of changes to accomplish a small thing. I think I'll stick with the elif for now.

I think my prefer approach would be to add _time_step_min and _time_step_max to every model and it's never inherited from the Performance and Cost base classes. Even for models that can only accept dt=3600.

Then we can remove the elif dt !=3600 because this will be caught in the above if statement.

I think we have a few different ways to essentially achieve the same checking capability here, thanks for the discussion. @kbrunik, I like your idea because of how explicit it is and how it will make it very clear what time steps each model accepts. I think this will make future updates to models as we expand out flexible dt values more trackable. I also think it will encourage (force) folks making new models to include _time_step_bounds, which will be useful to define at model creation time.

I'm done with those changes and welcome input from @jaredthomas68 and @elenya-grant! Are you okay with this? What would you change?

elenya-grant · 2026-04-07T16:20:58Z

h2integrate/core/h2integrate_model.py

+    def _check_time_step(self, model_name, model_object):
+        dt = int(self.plant_config["plant"]["simulation"]["dt"])
+
+        if hasattr(model_object, "_time_step_min") or hasattr(model_object, "_time_step_max"):


for some reason I'm wanting to suggest getattr instead (since you can provide defaults if it doesnt have the attribute) but I think that'd make the logic more sloppy. So - I like how you have it.

johnjasa

I like this PR! It is simple and helpful. I'd love to see your answers to @elenya-grant's questions as I had some similar thoughts.

For future work, as we build out more of the timestep handling and methods related to that, I could imagine a separate utilities file may be useful to reduce the size of h2integrate_model.py. I think as you work through other changes needed for this timestep capability, keep that in mind!

genevievestarke

Thank you for kicking this off, @jaredthomas68!! Just a few thoughts!

genevievestarke · 2026-04-09T19:59:05Z

h2integrate/converters/grid/grid.py


+    _time_step_min = 300
+    _time_step_max = 3600
+


I think making the min 300 seconds for now makes a lot of sense, since that's the smallest resource step that is available and also the smallest grid price data resolution. I think that letting the time step be 1 second is not realistic for the steady state models we have in H2I. This is something I feel pretty strongly about, but what are others' opinions about this?

genevievestarke · 2026-04-09T20:04:50Z

h2integrate/core/h2integrate_model.py

+    def _check_time_step(self, model_name, model_object):
+        dt = int(self.plant_config["plant"]["simulation"]["dt"])
+
+        if hasattr(model_object, "_time_step_min") or hasattr(model_object, "_time_step_max"):


I'm a little bit confused by this part. The if/else logic seems to use both values, but you're only checking for one or the other? Can a model just have one defined (i.e. only _time_step_max = 3600, and then any large step is fine)?

I see the confusion. I switched to using a tuple as suggested by @elenya-grant, which solves the odd logic. I also think that having an upper and lower bound is important, even if they are wildly different (e.g. 300 s, 1 week), just to make sure the behavior is reasonable.

jaredthomas68 · 2026-04-10T21:36:45Z

Thanks for introducing this PR! I had a couple of high-level suggestions/questions for other folks. I think this is the cleanest and least complicated way of introducing this flexibility - nice job!

Kinda big-picture consideration - should there be a valid time-step interval defined too? Like - for the grid, what if I tried to run a timestep of 563 seconds? Maybe thats fine but it seems weird. Should there be another parameter defined as like _time_step_interval? So that models can be run for timesteps:
valid_dts = np.arange(_time_step_min, _time_step_max, _time_step_interval)
(not asking for this to be implemented, but a question I think it worth considering)

I know this is still a draft, but I would like to see a test for the grid component using a smaller timestep.

Thanks for the review and questions. I think I would like to not include valid time step interval yet. We have essentially baked in a 1s increment. I think 563 s would be an odd time step, but there is nothing inherently wrong with it. If we do run into models that need to have an interval defined, it would not be difficult to include that capability at that point.

I have also added a unit test and an integration test for the variable dt grid component

…hourly dt

kbrunik

I like where this is headed! Thanks for a functional draft and the quick testing of the added capabilities. I think we will need a lot of rigorous testing around the cost modeling and financial modeling to ensure we are handling everything correctly (especially with feedstocks).

jaredthomas68 · 2026-04-10T23:32:52Z

I like where this is headed! Thanks for a functional draft and the quick testing of the added capabilities. I think we will need a lot of rigorous testing around the cost modeling and financial modeling to ensure we are handling everything correctly (especially with feedstocks).

Thank you @kbrunik, I don't think the cost and financial modeling will need much work because they use annual commodity amounts and capacity factor rather than the actual time-series. I looked into them but I don't think they need any changes.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds infrastructure to support non-hourly simulation time steps by validating dt compatibility per model via _time_step_bounds, and updates the grid models/docs/tests to demonstrate variable-dt behavior.

Changes:

Adds per-model timestep compatibility checks in H2IntegrateModel using _time_step_bounds.
Updates plant-config validation to allow variable dt as long as the simulated duration equals one year.
Extends grid performance/cost models and tests to handle non-hourly dt, plus documentation updates.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
h2integrate/core/test/test_framework.py	Adds unit tests covering the new per-model `dt` compatibility checks.
h2integrate/core/model_baseclasses.py	Stores `dt` on cost model base classes for use in variable-timestep cost calculations.
h2integrate/core/inputs/validation.py	Allows variable `dt` by validating `n_timesteps * dt == 1 year (seconds)`.
h2integrate/core/h2integrate_model.py	Introduces `_check_time_step()` and invokes it during model processing.
h2integrate/converters/grid/test/test_grid.py	Adds unit + integration tests exercising grid behavior at non-hourly `dt`.
h2integrate/converters/grid/grid.py	Declares timestep bounds for grid models and scales VarOpEx by `dt/3600`.
docs/technology_models/grid.md	Documents grid component timestep compatibility (5-min to 1-hour).
docs/intro.md	Notes that some models can now run at non-hourly time steps via bounds.
docs/developer_guide/adding_a_new_technology.md	Documents how model authors should specify `_time_step_bounds`.
CHANGELOG.md	Records the variable-`dt` infrastructure and grid bounds addition.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-11T00:12:01Z

h2integrate/core/h2integrate_model.py

        # Generalized function to process model definitions
        model_name = individual_tech_config[model_type]["model"]
        model_object = self.supported_models[model_name]
+
+        self._check_time_step(model_name, model_object)
+
        om_model_object = tech_group.add_subsystem(
            model_name,
            model_object(


_check_time_step() is only invoked through _process_model(). In create_technology_models() there are other code paths that instantiate models without calling _process_model() (e.g., the combined perf/cost model branch and the feedstock perf/cost models). Those models currently bypass the dt compatibility enforcement, which breaks the PR’s guarantee that all models in a simulation are compatible with the configured timestep. Recommendation (mandatory): ensure _check_time_step() is also called in those direct-instantiation branches, or refactor those branches to go through a single creation helper that always performs the check.

Copilot · 2026-04-11T00:12:02Z

h2integrate/core/inputs/validation.py

+    n_timesteps = plant_config["plant"]["simulation"]["n_timesteps"]
+    dt = plant_config["plant"]["simulation"]["dt"]
+
+    if int(n_timesteps * dt) != 31536000:  # seconds in simulation must be seconds/year
        msg = (
            "H2Integrate does not currently support simulations that are less than or "
            "greater than 1-year. Please ensure that "
-            "plant_config['plant']['simulation']['n_timesteps'] is set to 8760."
-        )
-        raise ValueError(msg)
-    if int(plant_config["plant"]["simulation"]["dt"]) != 3600:
-        msg = (
-            "H2Integrate does not currently support simulations with a time step that is "
-            "less than or greater than 1-hour. Please ensure that "
-            "plant_config['plant']['simulation']['dt'] is set to 3600."
+            "plant_config['plant']['simulation']['n_timesteps'] times "
+            "plant_config['plant']['simulation']['dt'] equals 31536000 (s)."
        )


The validation uses int(n_timesteps * dt), which truncates (floors) non-integer products and can incorrectly accept invalid configurations (or reject valid ones) if dt is not an integer type (e.g., 1800.5) or if n_timesteps * dt is a float with rounding error. Recommendation (mandatory): coerce n_timesteps and dt to integers first (or use exact equality on integers) and avoid truncation (e.g., compare int(n_timesteps) * int(dt) to the target, or use round(...) with an explicit tolerance if floats are truly allowed).

h2integrate/converters/grid/test/test_grid.py

docs/intro.md

Copilot · 2026-04-11T00:12:03Z

h2integrate/core/model_baseclasses.py

        )

+        # dt is seconds per timestep
+        self.dt = self.options["plant_config"]["plant"]["simulation"]["dt"]


dt is used arithmetically downstream (e.g., self.dt / 3600). To keep behavior consistent across configs and avoid accidental string/float surprises, it’s safer to normalize the stored value here. Recommendation (optional): cast to an int (or float if fractional seconds are intended) and consider naming it dt_seconds to make the unit unambiguous.

Suggested change

self.dt = self.options["plant_config"]["plant"]["simulation"]["dt"]

self.dt = float(self.options["plant_config"]["plant"]["simulation"]["dt"])

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

add draft for variable timestep framework

7d61a1c

jaredthomas68 requested review from RHammond2, elenya-grant, genevievestarke and johnjasa April 3, 2026 23:57

jaredthomas68 changed the base branch from main to develop April 3, 2026 23:57

elenya-grant reviewed Apr 7, 2026

View reviewed changes

jaredthomas68 and others added 2 commits April 7, 2026 13:50

Merge branch 'develop' into variabledt

3d44da6

Changed error wording

c36dfb8

johnjasa approved these changes Apr 9, 2026

View reviewed changes

Merge branch 'develop' into variabledt

852bc7f

genevievestarke reviewed Apr 9, 2026

View reviewed changes

jaredthomas68 added 5 commits April 10, 2026 14:55

Merge branch 'develop' into variabledt

f4b1fc8

Merge remote-tracking branch 'myfork/variabledt' into variabledt

352ed79

switch bounds to tuple

0774f31

minor test correction

1adab8c

include time-series generation functions

c74968e

jaredthomas68 added 2 commits April 10, 2026 16:35

add tests for variable dt and update simulation length check for non-…

935ef26

…hourly dt

Merge branch 'develop' into variabledt

58ebc70

kbrunik approved these changes Apr 10, 2026

View reviewed changes

jaredthomas68 added 2 commits April 10, 2026 17:18

update docs

82e8bff

update changelog

ee2a46f

jaredthomas68 added 3 commits April 10, 2026 17:40

update docs and doc strings

b9dc167

restore develop version of utilities.py

e866dd6

update dt bounds error

9eeec64

jaredthomas68 marked this pull request as ready for review April 11, 2026 00:02

jaredthomas68 requested review from elenya-grant and genevievestarke April 11, 2026 00:02

jaredthomas68 requested a review from Copilot April 11, 2026 00:04

Copilot AI reviewed Apr 11, 2026

View reviewed changes

Copilot started reviewing on behalf of jaredthomas68 April 11, 2026 00:39 View session

johnjasa and others added 9 commits April 11, 2026 00:20

Merge branch 'develop' into variabledt

f4e8701

Merge branch 'develop' into variabledt

d358b92

Apply suggestions from code review

edd178b

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Fixing tests

ac3ddcc

Adding time step bounds to all models

7334507

Adding time step bounds to the combiner/splitter/transporters

22b6cf7

Updates for tests

4b50db0

Adding time step bounds to the custom paper mill performance model

467caea

Added time step bounds to paper mill cost

5bc9b5e

	self.dt = self.options["plant_config"]["plant"]["simulation"]["dt"]
	self.dt = float(self.options["plant_config"]["plant"]["simulation"]["dt"])

Conversation

jaredthomas68 commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Framework to allow for variable 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 3: Related Issues

Section 4: Impacted Areas of the Software

Section 4.1: New Files

Section 4.2: Modified Files

Section 5: Additional Supporting Information

Section 6: Test Results, if applicable

Section 7 (Optional): New Model Checklist

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!

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 Apr 10, 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!

johnjasa left a comment

Choose a reason for hiding this comment

Uh oh!

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

jaredthomas68 commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbrunik left a comment

Choose a reason for hiding this comment

Uh oh!

jaredthomas68 commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaredthomas68 commented Apr 3, 2026 •

edited

Loading

kbrunik Apr 10, 2026 •

edited

Loading

jaredthomas68 commented Apr 10, 2026 •

edited

Loading