-
Notifications
You must be signed in to change notification settings - Fork 10
Trim grid to roads and measure completeness #276
base: develop
Are you sure you want to change the base?
Trim grid to roads and measure completeness #276
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.
I know you're still working on this but I added a few comments that I think can get cleaned up now. I'll review more later
:param grid_dto: the dto containing | ||
:return: geojson.FeatureCollection trimmed task grid | ||
""" | ||
trimmed_grid = GridService.trim_grid_to_aoi(grid_dto) |
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 don't think this function needs to additionally trim to the AOI. I see them as independent operations and that's why we have the independent functions. If we want to stack functionality, we can just stack the calls from the API. So from the API, we call trim_grid_to_aoi()
and use the output of that as the input for trim_grid_to_roads()
:)
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.
Fair enough. Done. Added a little todo for myself to do a pass over the comments and docstring later.
…T-Tasking-Manager into trim-grid-to-roads
…T-Tasking-Manager into trim-grid-to-roads
…T-Tasking-Manager into trim-grid-to-roads
@@ -432,3 +432,83 @@ def post(self): | |||
error_msg = f"IntersectingTiles GET API - unhandled error: {str(e)}" | |||
current_app.logger.critical(error_msg) | |||
return {"error": error_msg, "SubCode": "InternalServerError"}, 500 | |||
|
|||
|
|||
class ProjectActionsIntersectingRoadsAPI(Resource): |
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.
My thinking was to add this functionality into the ProjectActionsIntersectingTilesAPI
using some additional query params. For instance, add a bool param for trimToAOI
, trimToRoads
, filterWithWater
. Then call each service in series depending on the value of these bools. I would do an if
for each bool then nest a try:catch within so we can easily say at which stage it failed.
The idea here is to provide a single endpoint to call when the user clicks "Trim" and the levels of trimming just depends on the checkboxes they've selected.
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.
Ah ok. I just followed the previous examples from the previous tasking manager code. Surprisingly, none of the endpoints seem to use query params (request.args). So you want something like:
projects/actions/intersecting-roads/ (or /intersecting-tiles) => projects/actions/trim?method=[METHODS]
It might require a bit of refactoring on the frontend side as well. Currently, the trim to AOI only corresponds to a single toggle and endpoint (below).
lat_lon_arr = re.findall( | ||
r'"lat":\s+(-?\d+\.\d+),\s+"lon":\s+(-?\d+\.\d+)', overpass_resp.text | ||
) | ||
url = "https://tiles.mapillary.com/maps/vtp/mly1_public/2/{}/{}/{}?access_token={}".format( |
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.
this would also be better as an env var.
Additionally, let's use more explicit var names instead of url
twice :)
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 you do that? Something like this in the .env:
MAPILLARY_TILE_API_URL=https://tiles.mapillary.com/maps/vtp/mly1_public/2/{}/{}/{}?access_token={}
Where you can fill in the {}
with variables? Or are you talking about something like:
url = "{}/{}/{}/{}?access_token={}".format(
os.getenv("MAPILLARY_TILE_API_URL"),z, x, y, os.getenv("MAPILLARY_ACCESS_TOKEN")
)
backend/models/postgis/task.py
Outdated
if mapillary_roads: | ||
tasks = [ | ||
geojson.loads(task.geojson)["coordinates"][0][0] | ||
for task in project_tasks | ||
] | ||
overarching_bbox = MultiPoint( | ||
[(x, y) for task in tasks for x, y in task] | ||
).bounds | ||
x, y, z = bbox_to_tile(overarching_bbox) | ||
if z > 14: | ||
x, y, z = GridService._get_parent_tile(x, y, z) | ||
# if z < 14: | ||
# child_tiles = GridService._get_child_tile(x, y, z) # TODO Refactor so that it gives all 4 tiles | ||
# x, y, z = child_tiles[0] # arbitrarily pick the first one | ||
base_url = "https://overpass-api.de/api/interpreter?data=" | ||
url = ( | ||
base_url | ||
+ '[out:json][timeout:25];(way["highway"]{bbox};);out geom;'.format( | ||
bbox=( | ||
# Overpass is lon/lat | ||
overarching_bbox[1], | ||
overarching_bbox[0], | ||
overarching_bbox[3], | ||
overarching_bbox[2], | ||
) | ||
) | ||
) | ||
overpass_resp = requests.get(url) | ||
lat_lon_arr = re.findall( | ||
r'"lat":\s+(-?\d+\.\d+),\s+"lon":\s+(-?\d+\.\d+)', overpass_resp.text | ||
) | ||
url = "https://tiles.mapillary.com/maps/vtp/mly1_public/2/{}/{}/{}?access_token={}".format( | ||
z, x, y, os.getenv("MAPILLARY_ACCESS_TOKEN") | ||
) | ||
resp = requests.get(url) | ||
overarching_tile = mapbox_vector_tile.decode(resp.content) |
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.
There's technically a possibility here to get a not defined error since overarching_tile
is defined within the conditional and used in a lower conditional but it's not declared outside the conditionals.
My suggestion is to move the logic in this conditional into the one at L956. It doesn't look like there's actually a need to execute this before what happens below. Additionally, it would be good to move the logic from these conditionals into a function just to reduce the cyclical complexity of this function.
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.
Putting it into L956 would cause a lot of repeated work. That would be fetching all this data for each task since L944 is wrapped in a for task in project_tasks:
I'm trying to understand the overarching_tile
case you're describing. They're both wrapped up in the same conditional of if mapillary_roads:
so I don't think you can have them be mutually exclusive? Let me know if I'm missing something.
Changelog
Potential issues