Skip to content

Fix the local import of EnergySystem within nodes.py module#86

Draft
Bachibouzouk wants to merge 4 commits into
devfrom
fix/local-import-energy-system
Draft

Fix the local import of EnergySystem within nodes.py module#86
Bachibouzouk wants to merge 4 commits into
devfrom
fix/local-import-energy-system

Conversation

@Bachibouzouk

@Bachibouzouk Bachibouzouk commented May 26, 2026

Copy link
Copy Markdown
Contributor

Add a method connect_to_energy_system to the Node class to assign a reference to the EnergySystem when a Node instance is added to it and to connect Node class methods with the signal sent when from the Energysystem when the Node instance is added.

As now the nodes all have a _energy_system attribute, we can use it to
connect  `_add_subnodes` to the signal triggered when the node is added
to the energy system
@github-actions

Copy link
Copy Markdown

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/oemof/network
  energy_system.py
  src/oemof/network/network
  nodes.py 232
Project Total  

This report was generated by python-coverage-comment-action

@Bachibouzouk Bachibouzouk requested review from gnn, p-snft and uvchik May 26, 2026 13:46
@p-snft p-snft marked this pull request as draft May 26, 2026 13:54
@p-snft

p-snft commented May 26, 2026

Copy link
Copy Markdown
Member

I like the general idea. Note that if you allow telling a Node which EnergySystem it is part of, as part of the public API, this should also tell the EnergySystem of the Node.

For the naming, if we agree to add this to the API: Would a more classical property getter/ setter do? (I don't know how that works with signals.)

@p-snft p-snft changed the title Fixes the local import of EnergySystem within nodes.py module Fix the local import of EnergySystem within nodes.py module May 26, 2026

self._nodes.update(new_nodes)
for n in nodes:
self.signals[type(self).add].send(n, EnergySystem=self)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@gnn - I am not sure if there was a particular use of adding EnergySystem to the kwargs, I couldn't find any in the code, beside the test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is because @gnn did not want the Node to have a reference to the EnergySystem.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants