-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update study area to account for exclusion zones #5
Conversation
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.
Looks good, but some more tests would be good.
@@ -31,12 +32,7 @@ | |||
@click.argument("path_to_output") | |||
@click.argument("config", type=Config()) | |||
def retrieve_administrative_borders(path_to_countries, max_layer_depths, path_to_output, config): |
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 am not convinced by this configuration mechanism (which I introduced and) which we are extending here. But I guess it may not the right moment to fix that.
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.
Yeah, I didn't edit the config mechanism. Best to fix it universally in a different PR
@@ -78,6 +74,41 @@ def _country_features(path_to_file, layer_id, study_area): | |||
yield new_feature | |||
|
|||
|
|||
def _study_area(config): |
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.
Could this maybe move to utils.py
and have a few unit tests (especially including the error that you are catching)?
In that case I'd not hand the config over to it, but rather a bounds
object (probably a dict) and a exclusion zones
object (probably list of dicts).
@@ -38,10 +38,17 @@ scope: | |||
- "Serbia" | |||
- "Switzerland" | |||
bounds: | |||
x_min: -15.8 # in degrees east | |||
x_min: -30 # in degrees east |
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 you change the default.yaml
, I think you should test whether the entire workflow still works. Alternatively, create another config file. Even then, it would be good to test the workflow.
The package conflicts should be fixed at the moment.
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.
sure thing. I haven't checked the functioning of the workflow in this repo on any of these PRs, I have to admit...
@timtroendle I'm closing this in preference of #9, which also includes exclusion zone schema and tests |
Reopening PR from old repo: timtroendle/possibility-for-electricity-autarky#7
Fix for #1
For completeness, original PR text is below.
--
Now an arbitrary number of exclusion zones can be defined in the model scope. These can only be rectangular.
I can't test this on the whole workflow, since I have eternal package conflicts, but the defined 'atlantic islands' exclusion zone leads to the following study area:
If defining a polygon that fits within the exclusion zone (e.g.
small_hole = shapely.geometry.Polygon(((-25, 35), (-24, 35), (-24, 40), (-25, 40)))
), thenstudy_area.intersects(small_hole)
andstudy_area.contains(small_hole)
are both False.If defining a polygon that partially fits within the exclusion zone (e.g.
big_hole = shapely.geometry.Polygon(((-25, 35), (-10, 35), (-10, 45), (-25, 45)))
), thenstudy_area.intersects(big_hole) == True
andstudy_area.contains(big_hole) == False
.MWE