Distance to Ports #90
Conversation
MRossol
force-pushed
the
offshore_db
branch
2 times, most recently
from
1d992ca
to
df8fa0b
Compare
Codecov Report
@@ Coverage Diff @@
## master #90 +/- ##
==========================================
+ Coverage 78.93% 79.34% +0.41%
==========================================
Files 68 73 +5
Lines 7239 7553 +314
==========================================
+ Hits 5714 5993 +279
- Misses 1525 1560 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@grantbuster, this feature is "complete" and ready for review if you have time. A high level review is fine, but I would love your thoughts, inputs on the two similar run kwargs:
|
rename layer kwargs rename update kwarg to update_layer due to config method conflict
reVX/config/dist_to_ports.py
Outdated
| Parameters | ||
| ---------- | ||
| config : dict | ||
| Dictionary with pre-extracted config input group. |
There was a problem hiding this comment.
Can't this also be a filepath to config?
There was a problem hiding this comment.
good question, do you know? ;-) I can look
There was a problem hiding this comment.
haha yes, str filepath to json config or pre-extracted config in dict form
reVX/offshore/dist_to_ports.py
Outdated
| Parameters | ||
| ---------- | ||
| ports : str | ||
| Path to shape file containing ports to compute least cost distance |
There was a problem hiding this comment.
Why are ports a shape file? Shouldn't they just be lat/lon point locations?
There was a problem hiding this comment.
That's what I got from the GDS team, technically your correct. I'll make the parser more dynamic
There was a problem hiding this comment.
Yeah Im not surprised that they would have shape files inputs but it seems like all we need is point locations so like a meta data file might be worth handling.
reVX/offshore/dist_to_ports.py
Outdated
| pixel_coords = offshore_lat_lon[['latitude', 'longitude']].values | ||
| tree = cKDTree(pixel_coords) # pylint: disable=not-callable | ||
|
|
||
| port_coords = ports[['LATITUDE', 'LONGITUDE']].values |
There was a problem hiding this comment.
this seems like a bad hard-coded set of coordinate labels. Can that lat/lon label parser you wrote handle this?
There was a problem hiding this comment.
Good catch! Yes it should, that is a super handy new util!
| shore pixels set to 9999. | ||
| profile : dict | ||
| Profile (transform, crs, etc.) of arr raster | ||
| lat_lon : pandas.DataFrame |
There was a problem hiding this comment.
This docstring confuses me. How does the dataframe map the offshore pixel coordinates to the array indices? How would i use this object? What is the index of the dataframe? What are the columns? Are the offshore "pixels" the exclusion pixels or the wind resource pixels? This comment applies to a few places where this variables is passed around.
There was a problem hiding this comment.
this is all based on the exclusion arrays. This dataframe is a flattened list of the lat, lon, row, col coordinates for all offshore pixels. I'll make the docstrings clearer, you've really raised our docstring bar, I think its great but man are our old modules going to look bad ;-)
There was a problem hiding this comment.
Haha yes most of the bar raising is because i cant figure out how to use our old code....
reVX/offshore/dist_to_ports.py
Outdated
|
|
||
| class DistanceToPorts: | ||
| """ | ||
| Compute the distance to port in km |
There was a problem hiding this comment.
Maybe consider adding more description of the use case in the class docstring. For example, "use this class to compute the least cost distance from the exclusion grid pixels to port locations. The distance to shore layer will be used to calculate least cost paths around land masses and other such obstructions" or something like that?
And also maybe write down somewhere that you can update the least cost exclusion layer by adding in new ports? So like if you get one new port you can just run with a single input port and an existing least cost distnace layer? I think that is really powerful but kind of implicit right now.
| ports['row'] = pixels['row'].values | ||
| ports['col'] = pixels['col'].values | ||
| ports['dist_to_pixel'] = cls._haversine_distance(port_coords, | ||
| pixel_coords[idx]) |
There was a problem hiding this comment.
Is this mapping the ports to their respective nearest single 90m pixels? If so, should specify in docstring. Current statement of "port locations and their mapping to the offshore pixels" makes it sound like it could be port -> 1 or more offshore pixels.
There was a problem hiding this comment.
not quite, it is to the nearest offshore 90m pixel. The issue is that sometimes the ports sit inland based on our dist_to_coast raster
| saved, if None use the ports file-name, by default None | ||
| chunks : tuple, optional | ||
| Chunk size of dataset in .h5 file, by default (128, 128) | ||
| max_workers : int, optional |
There was a problem hiding this comment.
The parallelization scheme for this is by port right? So if adding one new port to the layer, max_workers should be 1?
There was a problem hiding this comment.
yes, but if your run with update_layer it could have multiple new ports in the shape file (or future .csv)
| @@ -354,7 +354,7 @@ def _haversine_dist(cls, plant_coords, sc_coords): | |||
| if plant_coords.shape[0] == 1: | |||
| dist = dist.flatten() | |||
|
|
|||
| R = 6373.0 # radius of the earth in kilometers | |||
| R = 6371.0 # radius of the earth in kilometers | |||
There was a problem hiding this comment.
I for the life of me could not figure out where you got 6373! Everything i found online said 6371, we should get consensus on that...
There was a problem hiding this comment.
Fortunately i think this is just a scalar on the distance so the relative and nearest distances should be all good. The radius depends on where you're measuring since the earth is not a perfect sphere. seems to be 6378 at the equator. But yes i'm also seeing 6371 as the average. Might have been a typo....
|
|
||
|
|
||
| @pytest.mark.parametrize('ports_layer', [None, 'test']) | ||
| def test_cli(runner, ports_layer): |
There was a problem hiding this comment.
I thought you were going to keep the heatmap plot output as an option in the test for this feature? This is definitely a feature thats easier to visualize.
There was a problem hiding this comment.
brain fart, I'll add in that function.
|
@MRossol what about |
|
input and output dist_layer works for me! |
|
@grantbuster I think I got all of the changes |
No description provided.