-
Notifications
You must be signed in to change notification settings - Fork 13
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
Feature/centroids as gdf #122
Changes from 40 commits
9ef492b
b083f5d
35a0d43
d267b14
0c4b73c
8f24efc
b50c54e
9725657
c4beb8c
23af185
ee173c3
1be80a8
bf47977
656920d
95d1177
d216be2
9257bf0
ad433f3
8fe2e35
6f0d924
a76aaa6
eac32d9
2c49b1f
7d41cc7
8e7c6f6
33f02a1
792dab4
034d363
156e572
1aefefe
9c52016
f28093d
9819929
4d6af11
b881190
8fe45be
0e8bc27
d6b3a5c
58bd3a7
d438c72
5f08ce0
c44c45b
0ada400
8624309
91e46f5
9f48d3f
999eb9a
1e41930
5fe87c5
116e2e8
16d5710
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,6 @@ | |
|
||
import logging | ||
import datetime as dt | ||
import copy | ||
from collections.abc import Iterable | ||
from pathlib import Path | ||
|
||
|
@@ -137,22 +136,22 @@ | |
if ISINatIDGrid: | ||
|
||
dest_centroids = RiverFlood._select_exact_area(countries, reg)[0] | ||
meta_centroids = copy.copy(dest_centroids) | ||
meta_centroids.set_lat_lon_to_meta() | ||
centroids_meta = dest_centroids.get_meta() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like the linter is not up-to-date with the latest version of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. The develop environment of climada_petals points to core package before the refactoring. Opposed to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we revert changes to this setup anyway, no worries ✌️ |
||
|
||
haz = cls.from_raster(files_intensity=[dph_path], | ||
files_fraction=[frc_path], band=bands.tolist(), | ||
transform=meta_centroids.meta['transform'], | ||
width=meta_centroids.meta['width'], | ||
height=meta_centroids.meta['height'], | ||
transform=centroids_meta['transform'], | ||
width=centroids_meta['width'], | ||
height=centroids_meta['height'], | ||
resampling=Resampling.nearest) | ||
x_i = ((dest_centroids.lon - haz.centroids.meta['transform'][2]) / | ||
haz.centroids.meta['transform'][0]).astype(int) | ||
y_i = ((dest_centroids.lat - haz.centroids.meta['transform'][5]) / | ||
haz.centroids.meta['transform'][4]).astype(int) | ||
haz_centroids_meta = haz.centroids.get_meta() | ||
x_i = ((dest_centroids.lon - haz_centroids_meta['transform'][2]) / | ||
haz_centroids_meta['transform'][0]).astype(int) | ||
y_i = ((dest_centroids.lat - haz_centroids_meta['transform'][5]) / | ||
haz_centroids_meta['transform'][4]).astype(int) | ||
|
||
fraction = haz.fraction[:, y_i * haz.centroids.meta['width'] + x_i] | ||
intensity = haz.intensity[:, y_i * haz.centroids.meta['width'] + x_i] | ||
fraction = haz.fraction[:, y_i * haz_centroids_meta['width'] + x_i] | ||
intensity = haz.intensity[:, y_i * haz_centroids_meta['width'] + x_i] | ||
|
||
haz.centroids = dest_centroids | ||
haz.intensity = sp.sparse.csr_matrix(intensity) | ||
|
@@ -166,14 +165,12 @@ | |
files_fraction=[frc_path], | ||
band=bands.tolist(), | ||
geometry=cntry_geom.geoms) | ||
# self.centroids.set_meta_to_lat_lon() | ||
else: | ||
cntry_geom = u_coord.get_land_geometry(countries) | ||
haz = cls.from_raster(files_intensity=[dph_path], | ||
files_fraction=[frc_path], | ||
band=bands.tolist(), | ||
geometry=cntry_geom.geoms) | ||
# self.centroids.set_meta_to_lat_lon() | ||
|
||
elif shape: | ||
shapes = gpd.read_file(shape) | ||
|
@@ -195,7 +192,6 @@ | |
haz = cls.from_raster(files_intensity=[dph_path], | ||
files_fraction=[frc_path], | ||
band=bands.tolist()) | ||
# self.centroids.set_meta_to_lat_lon() | ||
|
||
else: # use given centroids | ||
# if centroids.meta or grid_is_regular(centroids)[0]: | ||
|
@@ -206,8 +202,6 @@ | |
# (transform) | ||
# reprojection change resampling""" | ||
# else: | ||
if centroids.meta: | ||
centroids.set_meta_to_lat_lon() | ||
metafrc, fraction = u_coord.read_raster(frc_path, band=bands.tolist()) | ||
metaint, intensity = u_coord.read_raster(dph_path, band=bands.tolist()) | ||
x_i = ((centroids.lon - metafrc['transform'][2]) / | ||
|
@@ -321,7 +315,7 @@ | |
MemoryError | ||
""" | ||
self.centroids.set_area_pixel() | ||
area_centr = self.centroids.area_pixel | ||
area_centr = self.centroids.get_area_pixel() | ||
event_years = np.array([dt.date.fromordinal(self.date[i]).year | ||
for i in range(len(self.date))]) | ||
years = np.unique(event_years) | ||
|
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 see you updated the Jenkins scripts. Why was this addition necessary?
python -m pytest
as opposed topytest
adds the current directory to thesys.path
. This enables testing local source code of modules which are not installed. At the same time, it makes it less clear what code exactly is tested. I recommend making sure that Petals is installed (possibly from sources withpip install -e
), and then removingpython -m
, to make sure that the installed version is tested. See the GitHub Actions for reference, wherepython -m
was not required so farThere 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 addition is necessary on Jenkins because all the branches use the same conda environment.
pip install -e
would interfere with other branches' tests.For referring to an arbitrary climada core branch one can install that by creating a venv environment on top of the conda environment. (Which is about 10 times faster). But in such a layered environment
pytest
is taken from the underlying conda sources and imports climada from there, whilepython -m pytest
starts from venv and imports climada from here.This was done, e.g., for exactly this PR before #787 was merged.
GitHub Actions on the other hand build a new conda environment for every branch and commit. I'm afraid we don't have enough resources in terms of cpu or diskspace to do that on Jenkins.
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 still confused why this is necessary here in Petals but not in Core. Doesn't that mean there is something off with the environment configuration?
Anyway, I see that this is too much hassle to change. However, I would like to keep the default invoking of
pytest
for users, which should work best with the current installation instructions. I found a way to insert the different command into the Makefile, and will push it shortly.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.
Ok sorry I did not realize you added new bash scripts for Jenkins, see my comments there