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

Require label of Node to be given #43

Merged
merged 4 commits into from
Dec 19, 2023
Merged

Conversation

p-snft
Copy link
Member

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

We use labels for hashing of Nodes and also save them to a dict where the labels are used as keys. Thus, we implicitly assume that labels are constant. In my opinion, we should drop the default "None" from the labels, encouraging users to set them. Also, the label setter should be removed, as it is incompatible to the new dictionary approach.

@p-snft p-snft linked an issue Dec 19, 2023 that may be closed by this pull request
@p-snft p-snft self-assigned this Dec 19, 2023
@p-snft p-snft requested a review from a team December 19, 2023 10:34
@p-snft p-snft added the bug Something isn't working label Dec 19, 2023
@p-snft
Copy link
Member Author

p-snft commented Dec 19, 2023

I consider this a bugfix, as energy_system["label"].label == "label" can be false at the moment, which should not be the case.

@p-snft p-snft added this to the v1.0.0 milestone Dec 19, 2023
Copy link

@henhuy henhuy left a comment

Choose a reason for hiding this comment

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

Sounds reasonable. And changes LGTM.
Needs many adaptions in current oemof.solph environments and tests though.
Breaking change for oemof.solph

@p-snft
Copy link
Member Author

p-snft commented Dec 19, 2023

Sounds reasonable. And changes LGTM. Needs many adaptions in current oemof.solph environments and tests though. Breaking change for oemof.solph

Actually, this does not need any changes besides those because of #40. Explicitly not setting the label (label=None) still works, so not setting a label is still allowed when interfacing with solph. Only, changing a label is no longer possible - but as I said, it should not be as it can cause inconsistencies.

@p-snft p-snft merged commit ee63186 into dev Dec 19, 2023
12 checks passed
@p-snft p-snft deleted the fix/encourage-constant-labels branch December 19, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encourage (constant) labels
2 participants