-
Notifications
You must be signed in to change notification settings - Fork 50
Add BMT workflows and update functions to build Buildings module #433
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
base: main
Are you sure you want to change the base?
Conversation
- build based on context.material - default attributes saved in config.py and passed to context.material - mix-models material cli options overwrites defaul attributes - should be compatible for both cli build and workflow build - commented out for now
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #433 +/- ##
=======================================
- Coverage 75.5% 74.2% -1.4%
=======================================
Files 270 277 +7
Lines 22153 22407 +254
=======================================
- Hits 16740 16639 -101
- Misses 5413 5768 +355
🚀 New features to boost your workflow:
|
return s | ||
|
||
|
||
# same as build(), but context-based |
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.
Here and in .buildings.build, I think it would be preferable to adjust the build() or main() function to take a Context object, and then decide what to do/not do based on settings attached to it, like a .material.Config or .buildings.Config instance.
This way we could hopefully avoid duplicating methods, and reduce the risk that 2+ different 'build' methods diverge from one another.
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.
(As always, happy to try to help make this change.)
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, yes, that is what is supposed to be here.
(Duplicating the build()
or main()
for now simply to avoid touching something that so far works well.)
Let me list the requirements I can think of for now.
- The
build()
ormain()
are also called in CLI (e.g.,mix-models material-ix build --
), so it should still be able to collect CLI options that users put in. - When they are called in other workflows, they collect what is in the config (config appending to the original context) and build.
That is how I organize the duplicated build functions. It would be highly appreciated if you could help take a look at the duplicated builds when you have time (...it works but i feel my way is not the most smart/clean way...), or just point out the lines showing how transport build cover these.
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.
The key transport lines are:
message-ix-models/message_ix_models/model/transport/build.py
Lines 647 to 652 in b51d8f0
# Set up a Computer for input data calculations. This also: | |
# - Creates a Config instance | |
# - Generates and stores context.transport.spec, i.e the specification of the | |
# MESSAGEix-Transport structure: required, added, and removed set items | |
# - Prepares the "add transport data" key used below | |
c = get_computer(context, scenario=scenario, options=options) |
and:
message-ix-models/message_ix_models/model/transport/build.py
Lines 529 to 545 in b51d8f0
# Update .model.Config with the regions of a given scenario | |
context.model.regions_from_scenario(scenario) | |
# Ensure an instance of .transport.Config | |
if options is not None and "config" in options: | |
# Use an instance passed as a keyword argument | |
config = context.transport = options.pop("config") | |
if len(options): | |
raise ValueError( | |
"Both config=.transport.Config(...) and additional options={...}" | |
) | |
elif "transport" in context: | |
# Retrieve the current .transport.Config. AttributeError if no instance exists. | |
config = context.transport | |
else: | |
# Create a new instance using `kwargs` | |
config = Config.from_context(context, options=options) |
The latter seems complicated, but it is basically trying to cover for different CLI, testing, and other uses of the code, and also allow to selectively override one or a few Config settings. For a minimal implementation, you could try something like:
if "material" in context:
# Existing config instance, don't do anything
log.info(f"Use existing materials config: {context.material!r}")
else:
# There's no materials Config instance, create a new one
context["material"] = material.Config(**kwargs)
""" | ||
self.core.write_debug_archive() | ||
|
||
def print_contents(self): |
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.
Probably equivalent to:
print(repr(context.asdict()))
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.
ah, thx
This is a draft PR to track bmt development (issue #431) for the milestone "build the Buildings sector on a baseline scenario".
(Changes in B module, BMT module, (legacy report on message-data branch), tests, and docs, listing this as TODOs to be deleted later.)
How to review
mix-models bmt run --from="M solved" "B built"
to see if it works on an R12 scenario.mix-models bmt run --from="B solved" "B reported"
to see if the report works.build
function in the Buildings module should still work with the previous NAVIGATE scenarios but not sure how to test)PR checklist