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

Adding bus to parametrized elements iterative #35

Merged
merged 5 commits into from
Jun 28, 2023

Conversation

FelixMau
Copy link
Contributor

@FelixMau FelixMau commented Jun 28, 2023

Working towards #31
Adding busses by calling get_bus from Mapper iterative. Please confirm that each component within an Element should have the same busses.

Slightly related change: removed cls parameter from get_default and get bus since the class is stored in Mapper as adapter anyway.

Ps.: Sorry for missing pre-commit run.

Adding busses by calling get_bus from Mapper. Also removed `cls` parameter from get_default and get bus since the class is stored in Mapper as `adapter` anyway.
@FelixMau FelixMau requested review from henhuy and nailend June 28, 2023 10:42
@henhuy
Copy link
Contributor

henhuy commented Jun 28, 2023

PR looks good to me. Good catch that you removed cls from Mapper class!
But I don't understand your question:

Please confirm that each component within an Element should have the same busses.

Could you explain it again? thx

Copy link
Contributor

@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.

Mini request.
Rest looks very good IMO

data_adapter_oemof/build_datapackage.py Outdated Show resolved Hide resolved
@FelixMau
Copy link
Contributor Author

FelixMau commented Jun 28, 2023

PR looks good to me. Good catch that you removed cls from Mapper class! But I don't understand your question:

Please confirm that each component within an Element should have the same busses.

Could you explain it again? thx

Sure :) If one component ("each row within the scalars") within a Element would need different busses I would not be able to catch them because I call Mapper.get_busses() outside of the loop and with the last component.
I don't think this would be a problem and I've made that decision for a little runtime advantage and to not build large lists. The .get_bus function would not make any difference between the components. But if we would be changing this we could face some issues that are maybe not easy to debug?

@henhuy
Copy link
Contributor

henhuy commented Jun 28, 2023

If one component ("each row within the scalars") within a Element would need different busses I would not be able to catch them because I call Mapper.get_busses() outside of the loop and with the last component.

Oh okay. You are right - and indeed this could be the very case!
Thus, it would be good if you could gather busses for each row in a process and concat and unique them later...

@FelixMau FelixMau merged commit 867a722 into 31-write-datapackagejson Jun 28, 2023
@nailend nailend deleted the add_bus_elements branch July 6, 2023 14:40
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