-
Notifications
You must be signed in to change notification settings - Fork 207
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
Remove vresutil #1220
Remove vresutil #1220
Conversation
As a technical comment, the revised implementation for calculating Testing has resulted in non-zero differences for the old and new approaches, though the differences are less than 0.5% which is probably fine. To facilitate review, have added a comment block with the old implementation including vresutil part. |
@davide-f it would be great to have your review on that as it relates to the fixing the environment issues. Would be great to resolve it before the release |
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 PR looks great and CI passes.
It deserves adding a major release probably as the env is changed.
Fine for me then :) great catch!
scripts/build_population_layouts.py
Outdated
# ------------------------------------------------------------------------ | ||
# old implementation | ||
# ------------------------------------------------------------------------ | ||
# from functools import partial | ||
# from shapely.ops import transform, cascaded_union | ||
|
||
# def area(geom): | ||
# return reproject(geom).area | ||
|
||
# def reproject(geom, fr=pyproj.Proj(proj='longlat'), to=pyproj.Proj(proj='aea', lat_1=33., lat_2=72.)): | ||
# reproject_pts = partial(pyproj.transform, fr, to) | ||
# return transform(reproject_pts, geom) | ||
|
||
# in km^2 | ||
with mp.Pool(processes=snakemake.threads) as pool: | ||
cell_areas = pd.Series(pool.map(vshapes.area, grid_cells)) / 1e6 | ||
# with mp.Pool(processes=snakemake.threads) as pool: | ||
# cell_areas = pd.Series(pool.map(vshapes.area, grid_cells)) / 1e6 | ||
# ------------------------------------------------------------------------ |
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.
Can we drop this?
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 yes 😄 Done!
Thanks a lot @davide-f! The PR just adds a bit to the work you have done to accommodate the fixed environments 🙂 Have added a release note if you think the PR is worth it 😉 Agree, that is time for a major release. If CI will be still happy, I'll merge this PR and start preparations for that, as we discussed. Many thanks for the coordination and all the fantastic work you are doing. |
Closes #1211 replacing
vresutil
dependencies.To give some context,
vresutil
dependencies have been dropped with #803, but re-appeared due to flaws when merging the sector-coupled version.Changes proposed in this Pull Request
vresutil
toolset with shapely ones when calculating cell areas when building the population layouts.Checklist
envs/environment.yaml
anddoc/requirements.txt
.config.default.yaml
andconfig.tutorial.yaml
.test/
(note tests are changing the config.tutorial.yaml)doc/configtables/*.csv
and line references are adjusted indoc/configuration.rst
anddoc/tutorial.rst
.doc/release_notes.rst
is amended in the format of previous release notes, including reference to the requested PR.