Skip to content

Conversation

staudtMarius
Copy link
Member

Resolves #1226
Resolves #1310

@staudtMarius staudtMarius self-assigned this Jul 9, 2025
@staudtMarius staudtMarius added bug Something isn't working enhancement New feature or request labels Jul 9, 2025
Copy link
Member

@danielfeismann danielfeismann left a comment

Choose a reason for hiding this comment

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

Almost there :)

Copy link
Member

@danielfeismann danielfeismann left a comment

Choose a reason for hiding this comment

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

LGTM

@danielfeismann danielfeismann merged commit 2909188 into dev Jul 11, 2025
4 checks passed
@danielfeismann danielfeismann deleted the ms/#1226-change-SubgridContainer-to-be-in-line-with-the-galvanical-seperation branch July 11, 2025 14:46
@danielfeismann danielfeismann added this to the Version 8.0 milestone Jul 17, 2025
@danielfeismann
Copy link
Member

One thing we missed here is clarifying whether we consider the transformer to be part of the 'SubGridContainer', and if so, whether it is part of the lower or upper grid.

Excluding the transformer would make voltages within the container very simple and clear. However, this would mean that not all grid elements would be within the 'SubGridContainer'. I therefore tend to think that the connecting transformer should belong to the grid with the lower voltage level (but not the connecting node).

What do you think @sebastian-peter and @staudtMarius?

Once we agreed on a way we should update PSDM Docs and adapt OsmoGrid, which revealed this.

@sebastian-peter
Copy link
Member

@danielfeismann I think traditionally, we have (almost) handled it in the exact way you described. The transformer is supposed to be part of the lower sub grid. The upper connecting node of the transformer (which I think you meant there) is part of the sub grid as well, though, because otherwise we would have a connecting element (the transformer) for which one node (the upper) does not exist within the container.

Check out ContainerUtils#filterForSubnet for reference (filtering for transformer with lower node, but also adding the upper node):

Set<Transformer2WInput> transformer2w =
input.getTransformer2Ws().stream()
.filter(transformer -> transformer.getNodeB().getSubnet() == subnet)
.collect(Collectors.toSet());
/* Add the higher voltage node to the set of nodes */
nodes.addAll(
transformer2w.stream().map(Transformer2WInput::getNodeA).collect(Collectors.toSet()));

In my opinion, this approach is sufficiently well reasoned and should stay this way, however, there should definitely be documentation on this.

@danielfeismann
Copy link
Member

I see the point to avoid having a node of an asset in another container. But that is what we defined with our change, as the figure in our docs describe it.

I see these options

  1. keep the node outside - advantage; To me it looks that we can simplify the process in OSMoGrid a bit since we don't need to adapt nodes, subnet numbers etc.
  2. include it in the subgrid - no node missing but with the same reason one could argue that this one is missing in the upper grid. And we have again the topic of two voltages in a subgrid
  3. have the transformer excluded from the subgrids - see above
  4. have the node in both subgrids - might have new side quests, would like to avoid it.

@sebastian-peter
Copy link
Member

@danielfeismann I would opt for variant 4 (which might be how it's currently implemented). From a purely electrotechnical perspective, the upper node of a transformer is not part of the lower grid. However, to keep coherent data structures, it makes sense to include the node in both upper and lower sub grid.
Connecting elements (lines, transformers etc.) are the edges of our grid graph, and nodes are the vertices. In a sub graph, all edges are connecting two nodes each, but the crucial point is that both nodes have to exist. An edge (let's say a transformer) that connects node A and B, while only node B is part of the grid graph, is invalid.

@staudtMarius
Copy link
Member Author

@sebastian-peter Currently, the node exists in both containers, since the node might have some connected lines.

@danielfeismann We still have to do some adaptation, because if we change a node, we also have to update everything that is connected to the node.

One idea would be add some helper classes in OSMoGrid, where the transformers, lines, etc. don't reference the actual node object but rather its uuid. After everything is generated one could simply get the actual node object and build the final connector object.

@danielfeismann
Copy link
Member

danielfeismann commented Oct 15, 2025

Since it is a connecting element that would be my reason to opt for variant 3. You could see the Transformer as the connecting element. If you like to, see it as separate grid container with just the connecting element ("TransformerGrid") without any nodes. Then, the upper transformer node is part of the upper container, the lower of the lower, and the transformer simply connect them.

@ Marius: Sure we have to change somewhere something. I would prefer not to build the next helper / workaround method for this.

@danielfeismann
Copy link
Member

danielfeismann commented Oct 17, 2025

Alright, as per our discussion I adapted our rtd #1459. After I took a closer look at the changes in OSMoGrid it revealed that the real cause of the problem was the poor instantiation of the grid used in the tests.

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

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Misconfigured nodes lead to "loops not allowed" Fix issue when creating JointGridContainer with SwitchInput

3 participants