Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revision/explicit network kwargs #1032

Merged
merged 16 commits into from
Jan 12, 2024
Merged

Revision/explicit network kwargs #1032

merged 16 commits into from
Jan 12, 2024

Conversation

p-snft
Copy link
Member

@p-snft p-snft commented Dec 15, 2023

Adapt to explicitly named arguments in oemof.network. Will need to be merged before the next release of network due to oemof/oemof-network#40.

The changes also reveals some undiscovered bugs (mostly ignored
parameters). This commit does not address these. So, it is expected
that tests fail.
These labels were just ignored. With explicit kwargs, they cause errors.
At least in the latest versions, the keyword was just ignored.
With explicitly named arguments only, it causes errors.
This parameter was unused and will cause errors with explicit
keyword arguments.
The warnings need the network.Node to be initialised already.
@pep8speaks
Copy link

pep8speaks commented Dec 15, 2023

Hello @p-snft! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-12-22 12:57:10 UTC

p-snft and others added 3 commits December 15, 2023 21:07
To access e.g. .label, warn_ helpers need oemof.network.Entity to be
initialised first.
@p-snft
Copy link
Member Author

p-snft commented Dec 19, 2023

When using the branch aimed to be merged in oemof/oemof-network#40, I locally observe an issues I do not understand, yet:

tests/test_scripts/test_solph/test_lopf/test_lopf.py:112: in test_lopf
    om = Model(es)
src/oemof/solph/_models.py:386: in __init__
    super().__init__(energysystem, **kwargs)
src/oemof/solph/_models.py:151: in __init__
    self.flows = self.es.flows()
../network/src/oemof/network/energy_system.py:205: in flows
    return {
../network/src/oemof/network/energy_system.py:208: in <dictcomp>
    for target in source.outputs
E   AttributeError: 'ElectricalLine' object has no attribute 'outputs'

I do not see why this should be an error. ElectricalLine is a special Flow, and as such should not have outputs.

The label seems to be set to allow calling workaround
before Node is initialised.
ElectricalLine seems to be added to the EnergySystem
as if it was a Node. This, however, is no longer possible
as Flows do not have 'inputs'.
@p-snft
Copy link
Member Author

p-snft commented Dec 19, 2023

I do not see why this should be an error. ElectricalLine is a special Flow, and as such should not have outputs.

This is because the ElectricalLine is added to the EnergySystem. I changed this but also deactivated the test as the constraints are not built if it's not added as a Node. It's marked experimental, anyway.

@p-snft p-snft force-pushed the revision/explicit_network_kwargs branch from 646fb82 to f8131da Compare December 19, 2023 10:24
@p-snft p-snft marked this pull request as ready for review December 19, 2023 13:00
@p-snft
Copy link
Member Author

p-snft commented Dec 19, 2023

Coverage decreases because experimental ElectricalLine is no longer tested. (It is broken.)

@p-snft p-snft requested a review from a team December 22, 2023 13:01
@p-snft p-snft merged commit 3ad2fe2 into dev Jan 12, 2024
12 checks passed
@p-snft p-snft deleted the revision/explicit_network_kwargs branch January 12, 2024 09:23
@@ -117,8 +113,12 @@ def __init__(
label=label,
inputs=inputs,
outputs=outputs,
**custom_attributes,
custom_properties=custom_attributes,
Copy link
Contributor

@nailend nailend Mar 1, 2024

Choose a reason for hiding this comment

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

@p-snft why aren't these parameter-names aligned? I guess it started here. Wouldn't it be best to change in oemof.network to custom_attributes as well, especially as custom-attributes are frequently used in the oemof-tabular API?!

Copy link
Member Author

Choose a reason for hiding this comment

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

In Python, an attribute is a fixed term referring to a variable defined by the class in the scope of an object. The implementation that some though we had (but never had) in network added attributes

my_node = Node("name", some_feature= 5)  # API was never implemented
my_node.some_feature # "some_feature" is an attribute, API was never implemented
# 5

However, the API now is

my_node = Node("name", custom_properties={"some_feature": 5})
my_node.custom_properties["some_feature"]  # "some feature" is not an attribute
# 5

Copy link
Contributor

Choose a reason for hiding this comment

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

well custom_properties is an attribute though 😆

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.

3 participants