-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add facades CommodityGHG and ConversionGHG #180
Conversation
Failing test is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this feature @SabineHaas
I will try to get the time to see why the test of the infer_metadata method fails
I see that you just added documentation to the new features, which would have been a comment of the review :)
def init_emission_buses(self, kwargs): | ||
"""Adds emissions buses as output flows and drops them from kwargs""" | ||
for key, bus in list(kwargs.items()): | ||
if key.startswith("emission_bus"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Bus
would have a carrier
attribute, one could look for emission carriers, but it is not implemented in oemof.solph
as long as this naming convention is documented this is fine. Maybe an error or warning if no occurence of emission bus is found in kwargs would make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Bachibouzouk ! I have added warnings in 51bb272.
I also renamed bus
to value
to avoid missunderstandings in c0f2038 as the value can be of different types (solph.Bus, string) depending on the item.
f"'datapackage_{example_datapackage}.json' \n" | ||
+ open(script_path).read(), | ||
) | ||
if script_path.parts[-3] == "GHG": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for ignoring this test here? Time constraint most probably?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the infer test, that's failing tests/test_examples.py::test_examples_datapackages_scripts_infer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the implementation following my comments make sense and all test pass, congrats for the nice job :)
from .commodity import Commodity | ||
|
||
|
||
@dataclasses.dataclass(unsafe_hash=False, frozen=False, eq=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SabineHaas - Is there a reason why you used this instead of @dataclass_facade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a new issue for discussion - as I had this in my mind for quite a while:
#184
Description
Fixes #165 and #177
ConversionGHG
andCommodityGHG
which inherit ofConversion
andCommodity
marginal_costs
stay associated with the flows they are associated with inConversion
andCommodity
emission_bus_0
,emission_bus_1
etc.emission_factor_<label_of_emission_bus>
, e.g.:emission_factor_co2
if the bus the emission flows to is labeledco2
.Type of change
Please tick or delete options that are not relevant.
Checklist:
Please tick or delete options that are not relevant.
pre-commit
hooks