Skip to content

Bugfix: Connecting resource to multiple techs#655

Open
elenya-grant wants to merge 4 commits intoNatLabRockies:developfrom
elenya-grant:bugfix/resource_err
Open

Bugfix: Connecting resource to multiple techs#655
elenya-grant wants to merge 4 commits intoNatLabRockies:developfrom
elenya-grant:bugfix/resource_err

Conversation

@elenya-grant
Copy link
Copy Markdown
Collaborator

@elenya-grant elenya-grant commented Apr 7, 2026

Bugfix: Connecting resource to multiple techs

Added in a bugfix so a resource model can be connected to multiple technologies without throwing an error.

For example, if you have two different solar technologies (solar_farm_A, solar_farm_B) that are co-located, the below would've thrown an error saying theres missing resource models, even though there wasn't a missing resource model:

resource_to_tech_connections: 
- [site.solar_resource, solar_farm_A, solar_resource_data]
- [site.solar_resource, solar_farm_B, solar_resource_data]

This was because the resource_models dictionary built in connect_technologies() had keys of the format site_name-resource_model (ex: site-solar_resource) and this was being checked against the formatting in resource_to_tech_connections which uses a .. This check was only done if there were more resource to tech connections than resources defined for the site, which was not tested to handle this case.

The bugfix was just changing the notation for the resource_models dictionary to use a period instead of a dash.

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

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

  • h2integrate/core/h2integrate_model.py
    • connect_technologies(): changed the notation for the resource_models dictionary keys to use a period instead of a dash.
    • h2integrate/core/test/test_framework.py: added test to make sure that no error is thrown if connecting one resource to multiple technologies. New test is named test_no_resource_connection_error_resource_to_multiple_techs

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 elenya-grant requested review from johnjasa and kbrunik April 7, 2026 16:03
@elenya-grant elenya-grant added the ready for review This PR is ready for input from folks label Apr 7, 2026
Copy link
Copy Markdown
Collaborator

@johnjasa johnjasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun stuff! I like the enabling multiple techs and your nice test. I modified the test so the setup() methods are called (this would trigger the actual connection testing). One small non-blocking question for my own understanding!

Comment on lines +347 to +348
H2IntegrateModel(input_config)
assert True
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An important point here that I'm modifying but I want to draw attention to it -- to actually check this connection, we need to call final_setup() (usually done as part of run()) to actually test these connections.

for site_grp, site_grp_inputs in self.plant_config["sites"].items():
for resource_key, resource_params in site_grp_inputs.get("resources", {}).items():
resource_models[f"{site_grp}-{resource_key}"] = resource_params
resource_models[f"{site_grp}.{resource_key}"] = resource_params
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a small but impactful change but I can't quite place it since there aren't other changes related to this. Is this the simple bugfix (naming from - to .) and that's all that's needed to enable this? If so, love it! Just making sure I understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review This PR is ready for input from folks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants