Bugfix: Update storage models so can use different control types per storage type

[elenya-grant]

Bugfix: Update storage models so can use different control types per storage type

This is a bugfix that only applies to h2integrate/storage/storage_performance_model.py and h2integrate/storage/battery/pysam_battery.py.

The use-case where I encountered this example is not tested in H2I. But - basically I'm using a pyomo controller with the pysam battery and using the StoragePerformanceModel with the demand open-loop controller for hydrogen storage. This means that for the StoragePerformanceModel, I need the storage model to have the input hydrogen_set_point. I need the pysam battery to be input pyomo_dispatch_solver. The existing logic in these two controllers lead to a case where the hydrogen_set_point was not being added as an input to the hydrogen storage.

The code prior to this PR and how that logic was not working for the hydrogen storage is below:

        if "tech_to_dispatch_connections" in self.options["plant_config"]: # this is True, because of the battery
            self.tech_group_name = self.pathname.split(".")
            for _source_tech, intended_dispatch_tech in self.options["plant_config"][
                "tech_to_dispatch_connections"
            ]:
                if any(intended_dispatch_tech in name for name in self.tech_group_name):
                    self.add_discrete_input("pyomo_dispatch_solver", val=dummy_function) # this is not added as an input because the tech_to_dispatch_connections is for the battery
                    break

       else: # this loop isn't gone into because the if statement evaluated to true
            # using an open-loop storage controller
            self.add_input(
                "electricity_set_point",
                val=0.0,
                shape=self.n_timesteps,
                units="kW",
            )

New logic that works:

        using_feedback_control = False
        # 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
            self.tech_group_name = self.pathname.split(".")
            for _source_tech, intended_dispatch_tech in self.options["plant_config"][
                "tech_to_dispatch_connections"
            ]:
                if any(intended_dispatch_tech in name for name in self.tech_group_name):
                    self.add_discrete_input("pyomo_dispatch_solver", val=dummy_function)
                    # set the using feedback control variable to True
                    using_feedback_control = True
                    break

        if not using_feedback_control:
            # using an open-loop storage controller
            self.add_input(
                f"{self.commodity}_set_point",
                val=0.0,
                shape=self.n_timesteps,
                units="kW",
            )

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:

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

• Made Issue Refactor/Fix/Update tech_to_dispatch_connections #620 as part of this PR

Section 4: Impacted Areas of the Software

Section 4.1: New Files

• h2integrate/control/test/test_multistorage_pyomo_openloop_control.py: new tests to check that the introduced logic works as expected.

Section 4.2: Modified Files

• h2integrate/storage/storage_performance_model.py: updated logic for setting control input in setup()
• h2integrate/storage/battery/pysam_battery.py: updated logic for setting control input in setup()
• h2integrate/control/control_strategies/heuristic_pyomo_controller.py: (bugfix) moved setting the attributes of charge and discharge efficiency from setup() to initialize_parameters()
• h2integrate/control/test/test_heuristic_with_generic_storage.py: updated tech config to test that the bugfix in heuristic_pyomo_controller.py worked.
• h2integrate/control/control_strategies/storage/simple_openloop_controller.py: had to change strict=False when creating config to prevent errors when using the simple openloop controller in cases where the cost model and the performance model have shared parameters that are not shared with the openloop controller.

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

[genevievestarke]

Ok, so you're breaking the previous if/else into two if statements, based on whether feedback control is used or not, correct?
Could you just take away the "break" part of the loop? I'm assuming not, because the if loop will still be triggered by the pyomo tech to dispatch connection?

[jaredthomas68]

The logic here seems a little brittle. I'm ok with it for now, but if we keep the logic as is, can you make a test or two (one for each option in the logic maybe)?

One possible fix for the logic is to have a list of controllers that are or are not feedback controllers, or that do or do not need the electricity_set_point input. I don't really like that idea though because it is hard to maintain (like electricity_producing_techs).

[jaredthomas68]

The logic here seems a little brittle. I'm ok with it for now, but if we keep the logic as is, can you make a test or two (one for each option in the logic maybe)?

One possible fix for the logic is to have a list of controllers that are or are not feedback controllers, or that do or do not need the electricity_set_point input. I don't really like that idea though because it is hard to maintain (like electricity_producing_techs).

Just to clarify, I think tests are always important, but particularly in bug fixes.

[kbrunik]

I understand that this isn't the point of this PR but it illuminated to me that we have some weird logic in the plant_config. I vote that we potentially make an issue about it.

I think it's ambiguous that it says "tech_to_dispatch_connections" but excludes the open-loop controllers. I vote either we update this name to be more clear that this isn't used for open loop dispatch or we add all dispatch to the "tech_to_dispatch_connections". We also need to spell this out in documentation. It's currently not included.

[elenya-grant]

Made Issue #620

[kbrunik]

Thanks for adding all these tests! Great to see all the different combinations :)

[johnjasa]

Thanks for these changes! I think it's a step in a great direction. I appreciate you already adding tests based on Jared's feedback. I do think Kaitlin's suggestion for creating an issue for the tech_to_dispatch_connections is useful and reveals a gap in our documentation there already. Could you please create that issue and share your thoughts? Otherwise this is good to come in imo!

[jaredthomas68]

Looks good to me. Thanks for adding the tests and making the issue.