Add .jinja suffix; reorganise template rules
#409
Conversation
There was a problem hiding this comment.
The jinja part of this PR makes a lot of sense.
The part related to the "tech_locations_template" rules is nice in the sense that it removes all these rule dependencies. It comes at the price that "tech_group" and "tech" are merged into a single wildcard. That's less clear I find, but I didn't find a better solution either.
I definitely think the rules and wildcards should have different naming. I always takes my a while to understand what they are doing.
- "tech_and_group" -> "group_and_tech", because that's the order.
- I suggest to rename all "template" rules to "module" rules, because rules are typically named by their outputs (a model module in these cases) instead of their inputs (a template).
- "locations_template" -> "locations", or "locations_module"
- "techs_and_locations_template" -> "tech_module" (potentially plus "with(out)_location_data", see my comment)
- "bio_techs_and_locations_template" -> "biofuel_tech_module"
Snakefile
Outdated
| rule techs_and_locations_template: | ||
| message: "Create {wildcards.resolution} definition file for the {wildcards.tech_group} tech `{wildcards.tech}`." | ||
| message: "Create {wildcards.resolution} definition file for {wildcards.tech_and_group}." |
There was a problem hiding this comment.
Consider naming this "group_and_tech", highlighting that groups comes first.
There was a problem hiding this comment.
Also, a workflow-wide wildcard_constraint that requires this wildcard to contain / could be useful, right?
| @@ -58,7 +58,7 @@ | |||
| * **UPDATED** structure of YAML templates and parametrisation: | |||
| * Parametrisation moved to eurocalliopelib. | |||
| * Rules to parametrise split into smaller technology-specific rules, to ensure inputs are directly relevant to the files being parametrised. | |||
| * YAML templates restructured to match structure of final model (see `Updated (models) above`); | |||
| * YAML templates restructured to match structure of final model (see `Updated (models) above`) and given `.jinja` suffix to allow for IDE syntax highlighting (#404); | |||
There was a problem hiding this comment.
Do you use a plug-in for that? My VSCode seems to understand it's jinja (the icon is jinja), but I don't get any syntax highlighting or problems.
There was a problem hiding this comment.
Ok, found the answer myself, haha.
Snakefile
Outdated
| # For all cases where we don't have any location-specific data that we want to supply to the template | ||
| input: | ||
| template = techs_template_dir + "{tech_and_group}.yaml.jinja", | ||
| locations = rules.locations_template.output.csv |
There was a problem hiding this comment.
Unrelated to this PR: Should this rule be called locations instead of locations_template? I was misled into thinking this was a template, but it isn't.
There was a problem hiding this comment.
locations_from_template? I still like the link to templates in the rule name
There was a problem hiding this comment.
or locations_calliope_input?
There was a problem hiding this comment.
Personally, I like locations_module best. We already call these files modules and this would be most consistent. I am fine with renaming these files to something else, but I'd be consistent -- including in the docs.
Snakefile
Outdated
| script: "scripts/template_techs.py" | ||
|
|
||
| use rule techs_and_locations_template as dummy_tech_locations_template with: |
There was a problem hiding this comment.
consistent naming would be "dummy_techs_and_locations_template".
There was a problem hiding this comment.
Anyways, maybe different names would be more helpful in communicating the difference. I don't really understand why this is called a "dummy". Is it maybe: "tech_and_locations_template_with(out)_location_specific_data"? That seems the be the differentiating factor to me.
Or even more radical and better in my opinion: "model_component_with(out)_location_specific_data". We don't have a clear rule, but typically rules are named by (1) what they do or (2) what they produce, not by their inputs (here: template).
Or: "module_with(out)_location_specific_data". That's how we call these files in the documentation. My favourite right now.
Fixes #404
Also fixes
ruleorderissue highlighted in #394. I achieved this by merging{tech_group}/{tech}into{tech_and_group}which allows us to add wildcard constraints that are combinations of tech group and tech (currently, onlysupply/biofuel).Checklist
Any checks which are not relevant to the PR can be pre-checked by the PR creator. All others should be checked by the reviewer. You can add extra checklist items here if required by the PR.