More Checking on `technology_interconnections` by johnjasa · Pull Request #832 · NatLabRockies/H2Integrate

johnjasa · 2026-08-11T16:51:09Z

More Checking on `technology_interconnections`

This PR adds structured topology validation for technology_interconnections in H2IntegrateModel, raising descriptive errors for common configuration mistakes that previously went undetected until OpenMDAO setup or silently produced wrong results.

Summary of changes:

Add _validate_technology_interconnections method with three topology checks.
Extend tech_control_classifiers population to always run (was SLC-only).
Call the new validation in setup() before the existing commodity check.
Add integration tests covering all new error cases.

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 8: New Model Checklist (if applicable)

TODO:

Implement all three topology checks
Extend tech_control_classifiers to non-SLC models
Add tests for each new error case
Update CHANGELOG

Type of Reviewer Feedback Requested (on Draft PR)

Implementation feedback:
Confirm that the three topology checks (length-3 commodity pair detection, storage topology,
and general max-in/out) match the expected behavior described in the issue.

Section 3: General PR Checklist

Section 4: Related Issues

Solves #809

Section 5: Impacted Areas of the Software

Section 5.1: New Files

N/A

Section 5.2: Modified Files

h2integrate/core/h2integrate_model.py
- Add _validate_technology_interconnections method.
- Extend tech_control_classifiers population to non-SLC models.
- Call new validation in setup().
h2integrate/core/test/test_framework.py
- Add test_validate_technology_interconnections integration test.
- Update test_check_tech_interconnections second subtest to use a non-storage tech.

Section 6: Additional Supporting Information

The three checks implemented are:

Discouraged length-3 [commodity_out, commodity_in] connections - When a length-3 connection passes a list pair like ["hydrogen_out", "hydrogen_in"] where the prefixes match, a ValueError is raised prompting the user to use a length-4 connection with an explicit commodity and transport component.
Storage technology topology - For each storage technology in tech_control_classifiers: exactly 1 length-4 input is required; at most 1 length-4 output is allowed; and the direct upstream technology may have at most 2 output streams (one to storage, one to a combiner).
General max-in/out for non-special technologies - All other technologies connected via length-4 connections (excluding splitters, combiners, storage techs, and the direct upstream of storage techs) must have at most 1 input stream and 1 output stream. The storage upstream exemption applies only to the output-stream count; input streams are still checked.

The fix to always populate tech_control_classifiers (removing the self.slc guard) ensures checks 2 and 3 work correctly for non-SLC systems where storage is still present. All other usages of tech_control_classifiers were already guarded by if self.slc: so there are no unintended side effects.

Section 7: Test Results, if applicable

Validated with targeted test runs on h2integrate/core/test/test_framework.py and the full h2integrate/core/test/ suite. All failures in test_pose_optimization.py are pre-existing OpenMDAO API mismatches unrelated to this PR.

h2integrate/converters/ammonia/test/test_ammonia_synloop_model.py::test_size_mode_outputs

All targeted tests passed.

Section 8 (Optional): New Model Checklist

N/A

elenya-grant

really high-level initial review - thanks for addressing this issue! I think it'd be nice to see some tests with some "fake" systems that are more complex than is possible with models in H2I right now to test the limitations of this. For example, looking at systems with converter and storage technologies that have multiple input or output streams.

Some examples that come to mind are:

storage tech that has two input commodities (electricity and hydrogen)
converter that has two input commodities and two output commodities

I also think that the examples in H2I will do a good job of testing this functionality. Thanks John! Will re-review again later (I want to give it a deeper look)

elenya-grant · 2026-08-11T17:08:10Z

+        for source, dest, commodity in self.technology_graph.edges(data="commodity"):
+            if commodity is None:
+                continue  # length-3 connections carry no commodity; skip them
+            out_degs_l4[source] = out_degs_l4.get(source, 0) + 1
+            in_degs_l4[dest] = in_degs_l4.get(dest, 0) + 1


I'm wondering if theres a way to do this with dict(self.technology_graph.in_degree) - but then removing non 4-length connections. I can't imagine that it'd actually make the code any simpler - so I like your approach.

Thanks for this comment and context! I haven't changed anything here, but good to be aware.

elenya-grant · 2026-08-11T17:10:19Z

+                raise ValueError(
+                    f"Storage technology {storage_tech!r} has {n_in} input connection(s) in "
+                    f"the technology graph but should have exactly 1."
+                )


is there a future case where a storage technology would have 2 different commodity inputs? Like - power and hydrogen? If possible - it'd be nice to check that theres only one input connection per commodity input to the storage.

I think your code would be fine if (for some imaginary storage that takes electricity and hydrogen input) if the electricity and hydrogen come from the same upstream tech. I think that it'd cause an unnecessary error if electricity came from source_a and hydrogen came from source_b.

This is a good point for future cases; I modified the behavior of this check. Now, a storage tech receiving electricity from source_a and hydrogen from source_b now passes cleanly - two edges but each commodity has exactly one source. The only invalid case is when the same commodity arrives from two different technologies, e.g. hydrogen from both electrolyzer and h2_storage_bypass, when we should just use a combiner.

I agree - that sounds great! Thank you! I think thats perfect.

In PR #816, I added a fake system that handles another type of complex system I imagine we could have in H2I. I don't think that you have to use that specific fake system - but could be good to use something like that to test these checks for the connections into/out of a converter tech (h2integrate/control/control_strategies/system_level/test/test_slc_baseclass.py::test_find_converter_techs_fake_system)

elenya-grant · 2026-08-11T17:18:55Z

+            if "splitter" in tech or "combiner" in tech:
+                continue
+            if self.tech_control_classifiers.get(tech) == "storage":
+                continue  # already validated in check 2


I think we should change the _control_classifier for the splitter to be splitter and the _control_classifier for the combiner to be named combiner. This way - we can check the control classifier rather than having logic based on technology names (i.e., a splitter tech has to have "splitter" in the name and a combiner has to have "combiner" in the name)

Good call, I changed these two cases:
for combiner: "connector" ->"combiner"
for splitter: "connector" -> "splitter"

elenya-grant · 2026-08-11T18:04:11Z

 # this will naturally grow as we mature the interconnected tech
 technology_interconnections:
-  - [nuclear, htse, [heat_out, heat_in]]
+  - [nuclear, htse, heat, pipe]


can you use the generic transport model here instead? I think that we should have more examples showing that model because I think it'll be very useful as we develop new models with new commodity streams. I don't think we should add heat as a supposed transport item to pipe.

I've meant to make an issue about this - but I was talking to @genevievestarke a while back and we think that we should more examples using the generic transport model - and I think here is a good opportunity to do that.

elenya-grant

just left some small comments

elenya-grant · 2026-08-11T18:05:47Z

 # this will naturally grow as we mature the interconnected tech
 technology_interconnections:
-  - [nuclear, htse, [heat_out, heat_in]]
+  - [nuclear, htse, heat, pipe]


I've meant to make an issue about this - but I was talking to @genevievestarke a while back and we think that we should more examples using the generic transport model - and I think here is a good opportunity to do that.

elenya-grant · 2026-08-11T18:06:21Z

+        elif transport_item == "heat":
+            units = "kW"


see my comment earlier for example 36. I don't think we should add heat to the pipe right now.

johnjasa · 2026-08-11T19:21:57Z

Thanks for your comments, @elenya-grant! I've just removed heat from the pipe model and moved to use the generic transporter model in example 36. Also expanded the tests in test_framework.py.

elenya-grant

Just left some comments on some edge cases that I think are valid but would cause errors when they shouldn't. I can share the tests I made to check these if you want. I will finish this review tommorrow! Looks good so far! Only some unlikely (at the moment) edge-cases that I found that this would throw unnecessary errors for

elenya-grant · 2026-08-12T23:10:39Z

+            source_param, dest_param = connected_parameter
+            if not isinstance(source_param, str) or not isinstance(dest_param, str):
+                continue
+            if source_param.endswith("_out") and dest_param.endswith("_in"):


Not trying to nit-pick but I just remembered PR #774 and wonder if we should do:

if source_param.split("[")[0].endswith("_out") and dest_param.split("[")[0].endswith("_in"):

elenya-grant · 2026-08-12T23:16:13Z

+                        f"length-3 format. Use a length-4 connection instead: "
+                        f"[{source_tech!r}, {dest_tech!r}, {commodity_from_source!r}, "
+                        f"'<transport_tech>']. Use the generic transport component if "
+                        f"no specific transport is needed."


Could we rephrase this part of the error message: "Use the generic transport component if no specific transport is needed"?

I think that the generic transport component is useful if a) no specific transport is needed and b) it's a commodity that isn't supported by cable or pipe models (perhaps because its a new commodity). I think the most simple re-phrasing would be "You can use the GenericTransporterPerformanceModel to transport {commodity_from_source!r}"? But I'm open to other ideas

elenya-grant · 2026-08-12T23:39:34Z

+def test_validate_interconnections_multi_commodity_storage(subtests):
+    """Storage tech with two different-commodity inputs should be allowed.
+
+    Models a future storage that accepts both electricity (for charging) and
+    hydrogen (as the stored commodity) from two separate upstream technologies.
+    Each commodity has exactly one source, so no error should be raised.
+    """
+    interconnections = [
+        ["source_elec", "storage", "electricity", "cable"],
+        ["source_h2", "storage", "hydrogen", "pipe"],
+        ["storage", "h2_combiner", "hydrogen", "pipe"],
+    ]
+    classifiers = {
+        "storage": "storage",
+        "h2_combiner": "combiner",
+    }
+    fake = _make_fake_model(interconnections, classifiers)
+
+    with subtests.test("two-commodity storage passes validation"):


I know that this use-case is still not something we imagine happening a ton yet - but in my head, this should be a valid system of interconnections but an error is thrown:

interconnections = [ ["source_elec", "storage", "electricity", "cable"], ["source_h2", "storage", "hydrogen", "pipe"], ["storage", "h2_combiner", "hydrogen", "pipe"], ["source_h2", "h2_combiner", "hydrogen", "pipe"], ["source_elec", "elec_combiner", "electricity", "cable"], ["storage", "elec_combiner", "electricity", "cable"], ] classifiers = { "storage": "storage", "h2_combiner": "combiner", "elec_combiner": "combiner", }

Thanks for the example connections. The main difference here is that a singular storage technology accepts and stores both hydrogen and electricity, is that right? I don't immediately know of a physical system that behaves like that -- do you? I would argue we don't need to support this edge case at this moment, but let me know if you have a need or example case for it.

elenya-grant · 2026-08-12T23:52:03Z

+            for upstream_tech in upstream_techs_l4:
+                storage_upstream_techs.add(upstream_tech)
+                n_out_upstream = out_degs_l4.get(upstream_tech, 0)
+                if n_out_upstream > 2:
+                    raise ValueError(
+                        f"Technology {upstream_tech!r} feeds storage technology "
+                        f"{storage_tech!r} but has {n_out_upstream} output connection(s). "
+                        f"It should connect only to {storage_tech!r} and a combiner "
+                        f"(at most 2 output streams)."
+                    )


I think that here we should check that n_out_upstream based on the commodity

Suggested change

for upstream_tech in upstream_techs_l4:

storage_upstream_techs.add(upstream_tech)

n_out_upstream = out_degs_l4.get(upstream_tech, 0)

if n_out_upstream > 2:

raise ValueError(

f"Technology {upstream_tech!r} feeds storage technology "

f"{storage_tech!r} but has {n_out_upstream} output connection(s). "

f"It should connect only to {storage_tech!r} and a combiner "

f"(at most 2 output streams)."

)

for upstream_tech in upstream_techs_l4:

storage_upstream_techs.add(upstream_tech)

for commodity in self.technology_graph.edges[upstream_tech, storage_tech].get("commodity"):

n_out_upstream = out_commodity_dests.get(upstream_tech, {}).get(commodity,0)

if n_out_upstream > 2:

raise ValueError(

f"Technology {upstream_tech!r} feeds storage technology "

f"{storage_tech!r} but has {n_out_upstream} output connection(s). "

f"It should connect only to {storage_tech!r} and a combiner "

f"(at most 2 output streams)."

)

elenya-grant · 2026-08-12T23:58:29Z

+            n_out = out_degs_l4.get(storage_tech, 0)
+            if n_out > 1:
+                raise ValueError(
+                    f"Storage technology {storage_tech!r} has {n_out} output connection(s) in "
+                    f"the technology graph but should have at most 1."
+                )


Suggested change

n_out = out_degs_l4.get(storage_tech, 0)

if n_out > 1:

raise ValueError(

f"Storage technology {storage_tech!r} has {n_out} output connection(s) in "

f"the technology graph but should have at most 1."

)

for commodity, n_out in out_commodity_dests.get(storage_tech, {}).items():

if n_out > 1:

raise ValueError(

f"Storage technology {storage_tech!r} has {n_out} output connection(s) in "

f"of commodity {commodity} but should have at most 1."

)

johnjasa added 2 commits August 11, 2026 10:40

Initial version of additional tech interconnections checks

3a48625

Updated examples

67ce436

elenya-grant reviewed Aug 11, 2026

View reviewed changes

Addressing PR feedback

d4135a6

johnjasa marked this pull request as draft August 11, 2026 18:03

elenya-grant reviewed Aug 11, 2026

View reviewed changes

Used generic transporter instead of pipe for heat

9b6b5bd

johnjasa requested a review from elenya-grant August 11, 2026 19:22

johnjasa added 2 commits August 12, 2026 11:26

Updating examples

e314279

Merge branch 'develop' into add_tech_interconnection_checks

a9b9c59

elenya-grant requested changes Aug 13, 2026

View reviewed changes

johnjasa added 2 commits August 13, 2026 09:04

Robust slicer updates and error message improvement

e3ee095

Added Elenya's suggested code

83ff725

Uh oh!

Conversation

johnjasa commented Aug 11, 2026

More Checking on technology_interconnections

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 4: Related Issues

Section 5: Impacted Areas of the Software

Section 5.1: New Files

Section 5.2: Modified Files

Section 6: Additional Supporting Information

Section 7: Test Results, if applicable

Section 8 (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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

johnjasa commented Aug 11, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

More Checking on `technology_interconnections`