Bugfix: Connecting resource to multiple techs#655
Bugfix: Connecting resource to multiple techs#655elenya-grant wants to merge 4 commits intoNatLabRockies:developfrom
Conversation
johnjasa
left a comment
There was a problem hiding this comment.
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!
| H2IntegrateModel(input_config) | ||
| assert True |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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:This was because the
resource_modelsdictionary built inconnect_technologies()had keys of the formatsite_name-resource_model(ex:site-solar_resource) and this was being checked against the formatting inresource_to_tech_connectionswhich 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_modelsdictionary to use a period instead of a dash.Section 1: Type of Contribution
Section 3: General PR Checklist
docs/files are up-to-date, or added when necessaryCHANGELOG.md"A complete thought. [PR XYZ]((https://github.com/NatLabRockies/H2Integrate/pull/XYZ)", where
XYZshould 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.extensionmethod1: What and why something was changed in one sentence or less.Section 4.2: Modified Files
h2integrate/core/h2integrate_model.pyconnect_technologies(): changed the notation for theresource_modelsdictionary 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 namedtest_no_resource_connection_error_resource_to_multiple_techsSection 5: Additional Supporting Information
Section 6: Test Results, if applicable
Section 7 (Optional): New Model Checklist
docs/developer_guide/coding_guidelines.mdattrsclass to define theConfigto load in attributes for the modelBaseConfigorCostModelBaseConfiginitialize()method,setup()method,compute()methodCostModelBaseClasssupported_models.pycreate_financial_modelinh2integrate_model.pytest_all_examples.pydocs/user_guide/model_overview.mddocs/section<model_name>.mdis added to the_toc.yml