Framework to allow for variable dt

[jaredthomas68]

Framework to allow for variable dt

This PR contains a draft of 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 started work on adjustments to the grid components to work with variable dt to demonstrate how the framework changes will work with actual models.

Section 1: Type of Contribution

• Feature Enhancement
  • Framework
  • New Model
  • Updated Model
  • Tools/Utilities
  • Other (please describe):
• Bug Fix
• Documentation Update
• CI Changes
• Other (please describe):

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:
I am looking for input on the implementation and in particular corner cases that may cause problems I may not have caught.

Implementation feedback:

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

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

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

Section 4.2: Modified Files

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

Section 5: Additional Supporting Information

Section 6: Test Results, if applicable

Section 7 (Optional): New Model Checklist

• Model Structure:
  • Follows established naming conventions outlined in docs/developer_guide/coding_guidelines.md
  • Used attrs class to define the Config to load in attributes for the model
    • If applicable: inherit from BaseConfig or CostModelBaseConfig
  • Added: initialize() method, setup() method, compute() method
    • If applicable: inherit from CostModelBaseClass
• Integration: Model has been properly integrated into H2Integrate
  • Added to supported_models.py
  • If a new commodity_type is added, update create_financial_model in h2integrate_model.py
• Tests: Unit tests have been added for the new model
  • Pytest-style unit tests
  • Unit tests are in a "test" folder within the folder a new model was added to
  • If applicable add integration tests
• Example: If applicable, a working example demonstrating the new model has been created
  • Input file comments
  • Run file comments
  • Example has been tested and runs successfully in test_all_examples.py
• Documentation:
  • Write docstrings using the Google style
  • Model added to the main models list in docs/user_guide/model_overview.md
    • Model documentation page added to the appropriate docs/ section
    • <model_name>.md is added to the _toc.yml

[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]

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)

[elenya-grant]

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

[genevievestarke]

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?

[elenya-grant]

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.

[elenya-grant]

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]

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]

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)?