Update workflow to better align with euro-calliope #16

brynpickering · 2021-04-12T10:13:33Z

The idea here is to bring this workflow up-to-date with processes used in euro-calliope, such that making this a submodule of euro-calliope will be more straightforward.

Still to-do:

Split out all python/shell script rules so that the PYTHON identifier is no longer needed
Update dependencies
Run entire workflow on cluster to ensure it all still works!

timtroendle

This is a large change. Did you run it to see if the workflow still works?

Everything related to config is great!

I don't like that this change mixes the old mechanism and the new mechanism. I also don't like that we call the Python package "scripts" as this is confusing and may lead to wrong assumptions of users.

Fixing these things, however, would make this PR even larger and even more complex. So I guess it's fine to leave things as they are but we should, as a next step (in the next PR), introduce the library mechanism of euro-calliope too. All reused functions and definitions should then move to the library so that we minimise (better: get rid of) dependencies between "scripts". Then, we should also remove init.py and end up with an actual folder of scripts.

As soon as you've handled my comments, I'm fine with merging this.

Snakefile

rules/capacityfactors.smk

scripts/shared_coast.py

scripts/technically_eligible_capacityfactor.py

rules/potential.smk

scripts/administrative_borders.py

config/default.yaml

brynpickering · 2021-04-13T09:28:47Z

This is a large change. Did you run it to see if the workflow still works?

See my PR message, it's one of the to-dos!

I don't like that this change mixes the old mechanism and the new mechanism.

Also one of the to-dos, to separate out the few rules that currently call python from within a shell script, so that the python functions can be called with script: directly, then all old mechanism can go.

I also don't like that we call the Python package "scripts" as this is confusing and may lead to wrong assumptions of users.

Is there any real reason to have it be a Python package? To my mind the scripts within scripts are just that. Each rule calls a single file. There are occasional cross dependencies and there is an __init__.py, but it's more confusing to me to consider it a package than a collection of scripts accessed by the snakemake workflow.

Fixing these things, however, would make this PR even larger and even more complex. So I guess it's fine to leave things as they are but we should, as a next step (in the next PR), introduce the library mechanism of euro-calliope too. All reused functions and definitions should then move to the library so that we minimise (better: get rid of) dependencies between "scripts". Then, we should also remove init.py and end up with an actual folder of scripts.

This answers my comment above really, I like the sound of this approach in a new PR.

timtroendle · 2021-04-13T13:55:47Z

RE scripts/Python package: I think we are on the same line here. As long as there are dependencies between the "scripts", we need the Python package. But it should go as soon as we have the solarandwindpotentialslib.

Btw, maybe that lib (and the repo?) should be renamed to solarandwindandhydroandbiofuelspotentialslib at one point?

brynpickering · 2021-04-13T17:04:37Z

@timtroendle I just noticed that population.py seems to be unconnected from any rule - should the whole file be removed?

timtroendle · 2021-04-13T18:53:31Z

I removed the rule using this file in 5a77847 -- when cutting this repo out of my Home made study. That makes sense: population was only relevant for estimating electricity demand -- which is not part of this repo.

brynpickering · 2021-04-15T07:36:12Z

@timtroendle this is now ready to go. It builds every file required by all as well as the ninja input files. However, tests won't run. This is due to the current directory structure not fitting with the snakemake script functionality. I would handle this in another PR.

brynpickering · 2021-04-15T07:37:26Z

Btw, maybe that lib (and the repo?) should be renamed to solarandwindandhydroandbiofuelspotentialslib at one point?

Sounds perfect, if only there was a readily agreed upon group name for these technologies...

brynpickering · 2021-04-15T07:39:25Z

(btw, I know you said this could be merged once comments were addressed. The request for re-review is just to confirm that this can be merged with the test directory to be fixed at a later stage)

timtroendle · 2021-04-15T08:33:19Z

Sounds perfect, if only there was a readily agreed upon group name for these technologies...

technologieswhichdontrequirefossilfuelsandthereforedontemitgreenhousegasemissionslib?

timtroendle · 2021-04-15T08:34:28Z

(btw, I know you said this could be merged once comments were addressed. The request for re-review is just to confirm that this can be merged with the test directory to be fixed at a later stage)

Not very comfortable with that, but if you promise to tackle this soon, let's do it this way.

brynpickering · 2021-04-15T08:38:50Z

I think some more restructuring is needed to get tests working again, so I'd prefer not to bloat this PR further with it. Will sort soon, promise!

timtroendle

Not very comfortable with tests not running, but if you promise to tackle this soon, let's do it this way.

One thing I think should be changed is that you give Snakemake objects as inputs into almost all functions. Can you change that as I requested? (This is also in line with the way we do it in euro-calliope).

timtroendle · 2021-04-15T08:36:33Z

scripts/capacities.py

@@ -45,45 +29,45 @@ def potentials(path_to_units, path_to_eez, path_to_shared_coast,
    * allocate the offshore potentials to exclusive economic zones (EEZ),
    * allocate the offshore potential of EEZ to units based on the fraction of shared coast.
    """
-    with rasterio.open(path_to_eligibility_categories, "r") as src:
+    with rasterio.open(str(path_to_eligibility_categories), "r") as src:


I don't like this too much because this is a Snakemake detail. Rather solve it beofre calling this function.

snakemake.input.capacity_pv[0]

timtroendle · 2021-04-15T08:36:48Z

scripts/capacityfactors/averages_map.py

        ids = f_ids.read(1)
        meta = f_ids.meta
    averages = map_id_to_average_capacity_factor(ids, path_to_timeseries, meta["nodata"])
    meta["dtype"] = DTYPE
    meta["nodata"] = NODATA
-    with rasterio.open(path_to_output, "w", **meta) as f_avg:
+    with rasterio.open(str(path_to_output), "w", **meta) as f_avg:


timtroendle · 2021-04-15T08:39:35Z

Haha shit, seems I was too slow.

timtroendle · 2021-04-15T08:40:07Z

Can you mabye tackle this when you fix tests?

brynpickering · 2021-04-15T08:41:50Z

haha, will clean now! I actually preferred dealing with it at the final point, since there was otherwise mixing of when [0] would crop up. Sometimes it's in the snakefile, sometime's it's in the arguments being passed to the function in each python file. There should be some agreement on when we point to the first element of a 1-element snakemake NamedList object. Preferences?

timtroendle · 2021-04-15T08:43:05Z

True! Actually the cleanest is to have it in the Snakefile, no?

brynpickering · 2021-04-15T08:46:34Z

Sadly, I don't think you can do that in the snakefile unless you're pointing directly to another rule (i.e. it doesn't work with any outputs and it doesn't work with inputs that are defined as strings). The next available option is to put them all in the function call.

timtroendle · 2021-04-15T08:48:12Z

But all of this only happens when you use rule.xx.output, no? Otherwise inputs are strings in the first place.

timtroendle · 2021-04-15T08:52:40Z

It seems to me that euro-calliope handles this consistently (or almost consistently) wthin the Snakefiles (for inputs).

brynpickering · 2021-04-15T08:53:33Z

ok yeah. Maybe the cleanest option is to always name outputs. That way you can refer to that name when you use a rule output as an input and you can refer to that name when you refer to an output in the python files.

timtroendle · 2021-04-15T08:56:18Z

Maybe, yes. I don't have a strong opinion when it comes to naming or (int)-indexing.

Update workflow to better align with euro-calliope

a4cb2d3

brynpickering requested a review from timtroendle April 12, 2021 10:13

timtroendle approved these changes Apr 12, 2021

View reviewed changes

brynpickering added 3 commits April 14, 2021 18:32

Handle review comments; fix up code as necessary

f40be05

Revert change on LAU unzipping

9ed1c33

Final bug squashing

6238463

brynpickering requested a review from timtroendle April 15, 2021 07:36

brynpickering merged commit 3ca461b into main Apr 15, 2021

timtroendle approved these changes Apr 15, 2021

View reviewed changes

brynpickering mentioned this pull request Apr 15, 2021

Update all rules to handle single-element outputs #18

Merged

timtroendle deleted the align-to-euro-calliope branch April 15, 2021 12:01

brynpickering mentioned this pull request Apr 19, 2021

Move utils and relevant tests to own lib package #19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update workflow to better align with euro-calliope #16

Update workflow to better align with euro-calliope #16

brynpickering commented Apr 12, 2021

timtroendle left a comment

brynpickering commented Apr 13, 2021

timtroendle commented Apr 13, 2021

brynpickering commented Apr 13, 2021

timtroendle commented Apr 13, 2021

brynpickering commented Apr 15, 2021

brynpickering commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle commented Apr 15, 2021

timtroendle commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle left a comment

timtroendle Apr 15, 2021

timtroendle Apr 15, 2021

timtroendle commented Apr 15, 2021

timtroendle commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle commented Apr 15, 2021

timtroendle commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle commented Apr 15, 2021

Update workflow to better align with euro-calliope #16

Update workflow to better align with euro-calliope #16

Conversation

brynpickering commented Apr 12, 2021

timtroendle left a comment

Choose a reason for hiding this comment

brynpickering commented Apr 13, 2021

timtroendle commented Apr 13, 2021

brynpickering commented Apr 13, 2021

timtroendle commented Apr 13, 2021

brynpickering commented Apr 15, 2021

brynpickering commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle commented Apr 15, 2021

timtroendle commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle left a comment

Choose a reason for hiding this comment

timtroendle Apr 15, 2021

Choose a reason for hiding this comment

timtroendle Apr 15, 2021

Choose a reason for hiding this comment

timtroendle commented Apr 15, 2021

timtroendle commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle commented Apr 15, 2021

timtroendle commented Apr 15, 2021

brynpickering commented Apr 15, 2021

timtroendle commented Apr 15, 2021