Allow for connections including slice specifications by jaredthomas68 · Pull Request #774 · NatLabRockies/H2Integrate

jaredthomas68 · 2026-06-02T18:09:48Z

Allow connections using src_indices/slices

As noted in #763, there are times when it can be helpful to connect only part of a source variable to a target variable in the OpenMDAO connections. OpenMDAO allows for this, as described in the docs. This PR extends the connection specifications in H2Integrate to allow users to connect from part of an array variable to a target variable.

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

TODO:

Gather feedback on draft PR
Implement suggested changes
Add doc page
Add or update example using this feature

Type of Reviewer Feedback Requested (on Draft PR)

Structural feedback:

Implementation feedback:

I am mostly looking for feedback on the user interface for slice connections

Other feedback:

Section 3: General PR Checklist

Section 4: Related Issues

Fully resolves Connecting 1D outputs to ND inputs #763

Section 5: Impacted Areas of the Software

Section 5.1: New Files

h2integrate/core/test/test_split_indices.py: tests for _split_indices_from_connected_parameter_definition

Section 5.2: Modified Files

h2integrate/core/h2integrate_model.py
- _split_indices_from_connected_parameter_definition: parses connection definitions that include slices and returns the indices and the connection spec without the slice definition
- connect_technologies: use _split_indices_from_connected_parameter_definition to facilitate slice/src_indices connections
examples/15_wind_solar_electrolyzer/plant_config.yaml: include tech connections and finance models to demo different connection/finance approaches
examples/15_wind_solar_electrolyzer/run_wind_solar_electrolyzer.py: run and print results from 3 connection/finance approaches
examples/15_wind_solar_electrolyzer/tech_config.yaml: include grid_buy and feedstock to allow demonstration of different approaches
examples/test/test_all_examples.py: include tests for updated example 15

Section 6: Additional Supporting Information

Section 7: Test Results, if applicable

Section 8 (Optional): New Model Checklist

Copilot

Pull request overview

This PR extends the H2Integrate connection specification syntax to support OpenMDAO-style partial connections by allowing slice/index notation in connected parameter definitions and translating that into src_indices during connect_technologies().

Changes:

Adds _split_indices_from_connected_parameter_definition() to parse bracket slice/index specs and produce src_indices.
Updates connect_technologies() to pass src_indices into plant.connect(...).
Adds unit tests for slice parsing and loosens the plant config schema for technology_interconnections.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
h2integrate/core/h2integrate_model.py	Adds slice parsing and threads `src_indices` into technology connections.
h2integrate/core/inputs/plant_schema.yaml	Adjusts schema typing for `technology_interconnections` entries.
h2integrate/core/test/test_split_indices.py	Adds unit tests for `_split_indices_from_connected_parameter_definition`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

elenya-grant

Thanks for pulling this together to fast! It's looking good.I haven't checked whether you wanted feedback on this yet or not - so sorry for getting ahead of it if so! I think the tests for the standalone method look good - but I'm worried about some of functionality when it comes to actually running OpenMDAO.

My main comments/suggestions/questions on revisions of the code as-is are:

remove use of eval if possible
replace if/elif/else logic with if statements paired with a return within that if statement.
why not use re.search(r"\[.*?\]", dest_parameter) instead of defining a pattern variable?
Nitpick and non-blocking: _split_indices_from_connected_parameter_definition() is a long method name. It's very clear what it does but I wonder if theres a shorter name that would work?

I'm assuming that these are on your to-do list, but some to-dos I'm thinking of are:

Integration tests where openmdao is actually run.
Updates to docs/user_guide/connecting_technologies.md
Added error messages when someone tries to use this incorrectly.

elenya-grant · 2026-06-02T22:52:29Z

+
+            # Scale source indices by destination length to handle shape mismatches
+            # Example: source_slice="[0:1]" with dest_length=8760 -> multiply 1 by 8760
+            src_indices = eval(f"om.slicer[{source_slice} * {dest_length}]")


why do we need to use the om.slicer here? I think in the past we've tried to stay away from the use of eval.

As this is, I don't think this would work if someone was using more than 1 slice from the source when open-mdao is actually run. Aka - if src_slice is len 2 - then we don't want to multiply it by the full destination length (because then it'll be twice as long as needed).

Like this case:

source_parameter = "power_in[0,1,2]" dest_parameter = "power_out[:8760]"

Then shouldn't we take the [0,1,2] and multiply it by 2920 instead of 8760?

We definitely need om.slicer to properly format for openmdao. I have dropped the use of eval. I updated the logic to allow for tiling as you suggested.

You are right about eval. I removed eval. I kept om.slicer because it is an easy way to create the slices in the way that openmdao expects. I also added the ability to tile as you suggested.

yeah - I like the use of om.slicer (once I understood what it was doing). Thanks for getting rid of eval()

elenya-grant · 2026-06-02T23:21:06Z

+            pattern = re.compile(r":(\d+)")
+            dest_length = int(pattern.search(dest_slice).group().split(":")[-1])


Suggested change

pattern = re.compile(r":(\d+)")

dest_length = int(pattern.search(dest_slice).group().split(":")[-1])

if (d_start_index := re.search(r"(\d+):", dest_slice)) is not None:

raise ValueError(f"Cannot have starting index of {d_start_index.group()} in parameter {dest_parameter}")

dest_indices = re.search(r":(\d+)", dest_slice)

if (dest_indices := re.search(r":(\d+)", dest_slice)) is not None

dest_length = int(pattern.search(dest_slice).group().split(":")[-1])

I think this has been addressed, but with a different approach. Let me know if you still think this should change.

…ce and connection approaches

…ectrolyzer example

jaredthomas68 · 2026-07-10T16:23:03Z

Thanks for pulling this together to fast! It's looking good.I haven't checked whether you wanted feedback on this yet or not - so sorry for getting ahead of it if so! I think the tests for the standalone method look good - but I'm worried about some of functionality when it comes to actually running OpenMDAO.

My main comments/suggestions/questions on revisions of the code as-is are:
* remove use of `eval` if possible

* replace `if/elif/else` logic with `if` statements paired with a `return` within that if statement.

* why not use `re.search(r"\[.*?\]", dest_parameter)` instead of defining a `pattern` variable?

* Nitpick and non-blocking: `_split_indices_from_connected_parameter_definition()` is a long method name. It's very clear what it does but I wonder if theres a shorter name that would work?
I'm assuming that these are on your to-do list, but some to-dos I'm thinking of are:
* Integration tests where openmdao is actually run.

* Updates to `docs/user_guide/connecting_technologies.md`

* Added error messages when someone tries to use this incorrectly.

I have added two more finance/connection approaches to example 15 (wind solar electrolyzer) and tests.

I updated the connecting_technologies.md file to include documentation for using slice notation in connections.

I added a check to catch incorrect usage and provide what I hope is a clear error message.

elenya-grant

I left a few clarifying questions but the logic is a lot easier to follow now! Thanks for working on this! Looks good to go on my end!

elenya-grant · 2026-07-10T17:51:56Z

+
+Behavior depends on which side carries a slice:
+
+- **Source only** (`"source_param[0:100]"`): the slice selects source indices directly (supports start/stop/step, e.g. `[0:100:2]`, and `[:]` for the full range).


Clarification question: When it's "Source only", then the destination length isn't needed? Does this mean that if my source_param is a 100 element array and my destination is a 50 element array, then I just need to do the connection as
["source_tech", "destination_tech", ["source_param[0:100:2]", "destination_param"]]

Yes, that is correct.

elenya-grant · 2026-07-10T17:53:54Z

+Behavior depends on which side carries a slice:
+
+- **Source only** (`"source_param[0:100]"`): the slice selects source indices directly (supports start/stop/step, e.g. `[0:100:2]`, and `[:]` for the full range).
+- **Destination only** or **both sides equal**: no `src_indices` are applied (the slice is treated as documentation of the target shape).


Idk if the definition of "destination only" needs to be explained here? Isn't "Destination only" when we don't specify indices?
["source_tech", "destination_tech", ["source_param", "destination_param"]]

Yes, that is what it means by "no src_indices are applied". Basically it treats it as though no slice was provided.

improve comment Co-authored-by: elenya-grant <116225007+elenya-grant@users.noreply.github.com>

move tests for split indices

e117db0

jaredthomas68 requested review from Copilot, elenya-grant and johnjasa June 2, 2026 18:09

jaredthomas68 added enhancement New feature or request framework ready for review This PR is ready for input from folks labels Jun 2, 2026

Copilot started reviewing on behalf of jaredthomas68 June 2, 2026 18:10 View session

jaredthomas68 changed the title ~~move tests for split indices~~ Allow for connections including slice specifications Jun 2, 2026

jaredthomas68 mentioned this pull request Jun 2, 2026

Connecting 1D outputs to ND inputs #763

Open

Copilot AI reviewed Jun 2, 2026

View reviewed changes

Comment thread h2integrate/core/h2integrate_model.py

Comment thread h2integrate/core/h2integrate_model.py Outdated

Comment thread h2integrate/core/inputs/plant_schema.yaml Outdated

Comment thread h2integrate/core/test/test_split_indices.py Outdated

revert obsolete changes to plant schema

5866469

elenya-grant reviewed Jun 2, 2026

View reviewed changes

Comment thread h2integrate/core/h2integrate_model.py Outdated

elenya-grant reviewed Jun 2, 2026

View reviewed changes

Comment thread h2integrate/core/h2integrate_model.py Outdated

elenya-grant reviewed Jun 2, 2026

View reviewed changes

johnjasa added the needs modifications This PR has been reviewed, at least partially, and is ready for PR author response label Jun 11, 2026

jaredthomas68 removed the ready for review This PR is ready for input from folks label Jun 11, 2026

elenya-grant mentioned this pull request Jul 6, 2026

Multi-year sell-price in ProFAST NPV #799

Draft

27 tasks

jaredthomas68 added 9 commits July 9, 2026 11:45

Merge branch 'develop' into src_indices

986b536

fix bugs in length 3 connections for src_indices

8f8d5b0

remove use of eval

700db44

add ValueError for non-zero destination slice starts

f414c21

allow for src_indices tiling into destination

fcd3e02

update wind solar electrolyzer example to demonstrate different finan…

3ca1e97

…ce and connection approaches

add tests for each finance/connection approach added to wind solar el…

2c85261

…ectrolyzer example

add documentation for variable slicing in technology connections

a6c53cf

update changelog

97f0fd6

jaredthomas68 marked this pull request as ready for review July 10, 2026 16:31

jaredthomas68 requested a review from elenya-grant July 10, 2026 16:32

fix typo

5fc2abd

elenya-grant approved these changes Jul 10, 2026

View reviewed changes

Update docs/user_guide/connecting_technologies.md

3ebf8e2

improve comment Co-authored-by: elenya-grant <116225007+elenya-grant@users.noreply.github.com>

jaredthomas68 requested review from johnjasa and removed request for johnjasa July 10, 2026 18:26

		pattern = re.compile(r":(\d+)")
		dest_length = int(pattern.search(dest_slice).group().split(":")[-1])

-            pattern = re.compile(r":(\d+)")
-            dest_length = int(pattern.search(dest_slice).group().split(":")[-1])
+            if (d_start_index := re.search(r"(\d+):", dest_slice)) is not None:
+               raise ValueError(f"Cannot have starting index of {d_start_index.group()} in parameter {dest_parameter}")
+            dest_indices = re.search(r":(\d+)", dest_slice)
+            if (dest_indices := re.search(r":(\d+)", dest_slice)) is not None
+               dest_length = int(pattern.search(dest_slice).group().split(":")[-1])


		Behavior depends on which side carries a slice:

		- Source only (`"source_param[0:100]"`): the slice selects source indices directly (supports start/stop/step, e.g. `[0:100:2]`, and `[:]` for the full range).

Uh oh!

Conversation

jaredthomas68 commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Allow connections using src_indices/slices

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elenya-grant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredthomas68 commented Jul 10, 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jaredthomas68 commented Jun 2, 2026 •

edited

Loading