generate key in function #48
Conversation
spec_creation/autofill_eval_spec.py
Outdated
|
|
||
| # key that holds information regarding timespan of ground truth data | ||
| # TODO: determine where to find timespan info for trip | ||
| t["timespan"] = "" |
There was a problem hiding this comment.
This is roughly what I had in mind. But I think that the timespan needs to be at the section/leg level, and not the trip level. The commit I made yesterday has an example of how a user might specify the valid timespan.
d647389#diff-babc6ee3762947af73e5aaae138f9c22badd4b75cd4c84b06d7cd0017ac36f62R397
So there are two "Wurster bikeshare station" entries and each of them is valid for some time range.
Note that the "Wurster bikeshare station" entries are in the leg and not the trip because the rest of the trip is not necessarily modified.
There was a problem hiding this comment.
But the copy code seems like it would be fairly similar
There was a problem hiding this comment.
Added a new commit, let me know if my updates reflect what you have in mind!
There was a problem hiding this comment.
wait, why is there only one timespan in the filled leg?
shouldn't there be one for each leg?
Also, what did you think that the format of the timespan would look like?
it can't be a single duration number because then you don't know when that duration happened
if it is a range, would you have to parse it?
which is why I have valid_start_ts and valid_end_ts in my proposal
spec_creation/autofill_eval_spec.py
Outdated
| @@ -308,6 +308,11 @@ def validate_and_fill_eval_trips(curr_spec): | |||
| # Let's check again after we have inserted the shim legs | |||
| assert not has_duplicate_legs(t), \ | |||
| "Found duplicate leg ids in trip %s" % t["id"] | |||
|
|
|||
There was a problem hiding this comment.
it seems like there should be one timespan entry for each l in ret_leg_list
|
looks a lot better.
|
… this method to add temporal ground truths to start locs, shim legs, and trajectories
|
my last commit should address those comments, take a look when you get time @shankari |
There was a problem hiding this comment.
Looks a lot better. Now you just have to fill in the values as we discussed yesterday:
- override-specified values if they exist
- spec-wide values if they don't
Have you pulled from new_spec_edits yet? the ucb_mtv timeline has some overriden entries so you should be able to test the fill code against that.
spec_creation/autofill_eval_spec.py
Outdated
| @@ -156,9 +156,28 @@ def get_route_from_relation(r): | |||
| return get_coords_for_relation(r["relation_id"], | |||
| r["start_node"], r["end_node"]) | |||
|
|
|||
| def _add_temporal_ground_truth(orig_loc): | |||
| # fill in timespan for which ground truth is valid (see issue #11) | |||
| # first, if start_loc/end_loc are dicts, we need to convert end_loc to a list of dicts. | |||
There was a problem hiding this comment.
nit: in the second instance, not only end_loc, both
…in logic to fill in valid_start_fmt_date
|
ran into a couple issues while working on the new commit:
also, there were a few escape characters that weren't fully escaped in |
|
Addressing the issues in order:
short answer: you need to set the values in An alternative is to just specify polylines directly. Ask users to get polylines from the API (maybe using What do you think? |
ah this should probably be |
I spot-checked a couple of those, and it looks like I manually created those polylines by encoding the coordinates from the filled spec. I don't see them in either I wish I had taken better notes on how I was making the changes, but I didn't think I would return to them almost a year later. edit:
|
spec_creation/autofill_eval_spec.py
Outdated
| # next, add dates if they do not exist | ||
| for l in loc: | ||
| if not l["properties"].get("valid_start_fmt_date"): | ||
| l["properties"]["valid_start_fmt_date"] = start_fmt_date |
There was a problem hiding this comment.
We haven't finished discussing the design completely, but I think that the filled spec should store timestamps instead of a formatted string.
This related to the final step in the issue:
Change the code that lookup up the ground truth for the evaluation (in emeval/input/spec_details.py - e.g. get_ground_truth_for_leg)
Recall that we will need to retrieve the ground truth in get_ground_truth_leg to evaluate the accuracy. It will be much easier to retrieve the correct ground truth object if we can compare timestamps directly.
|
thanks for the comments -- would you be able to meet at our pencilled in 10am ET time tomorrow morning to go over them? had a few questions regarding trajectories, ORSM, and your point about design. sorry if this is late! |
Just now seeing that the 10am slot was only penciled in for last week -- I will follow up with you during our usual 1pm time. Thanks! |
|
incorporated the suggestions I didn't have questions about in my most recent commit. For now I am copy-pasting the following lines into OSRM_HOST = "https://routing.openstreetmap.de"
OSRM_ROUTES = {
"CAR": "routed-car/route/v1/driving",
"WALKING": "routed-foot/route/v1/driving",
"BICYCLING": "routed-bike/route/v1/driving",
"BUS": "routed-car/route/v1/driving"
}though I end up running into the error: Traceback (most recent call last):
File "autofill_eval_spec.py", line 394, in <module>
eval_spec = validate_and_fill_eval_trips(calib_spec)
File "autofill_eval_spec.py", line 331, in validate_and_fill_eval_trips
ret_leg_list.append(validate_and_fill_leg(l, default_start_fmt_date, default_end_fmt_date))
File "autofill_eval_spec.py", line 238, in validate_and_fill_leg
"coordinates": [coords_swap(rc) for rc in route_coords]
UnboundLocalError: local variable 'route_coords' referenced before assignmentWill ask you about the OSRM issues at 1 -- it was working fine with the |
|
saw that the path to |
|
in general, during the evaluation, we read the actual sensed data from the e-mission server (cardshark -> localhost). We also read the ground truth spec from the e-mission server (cardshark -> localhost). This is the part in the notebooks about if you dump out sdunp, you will find the same filled spec that it checked in to the repo ( then, when we want to compare whatever analysis we have done against the ground truth to generate metrics, we need to get the ground truth for each leg. We do this by traversing the tree of sensed data in all its configurations and then finding the ground truth each leg. Note that the ground truth does not depend on the sensing configuration, only the leg of the trip. e.g. in Evaluate_power_vs_motion_activity.ipynb in particular Now, the ground truth depends on the time at which the data was collected. So you need to change the implementation of the This will not affect the ground truth generation, which is what filling the spec does. This affects ground truth lookup during evaluation. To test it, there are no automated unit tests, unfortunately. We need to test two things:
|
|
wrt the route_coords error, looking at the if statement before it, I wonder if you are running into this branch. you could add some logging to double-check. a fix might be to assign the |
|
On line 238, should we be iterating over |
|
Getting the error Traceback (most recent call last):
File "autofill_eval_spec.py", line 395, in <module>
eval_spec = validate_and_fill_eval_trips(calib_spec)
File "autofill_eval_spec.py", line 332, in validate_and_fill_eval_trips
ret_leg_list.append(validate_and_fill_leg(l, default_start_fmt_date, default_end_fmt_date))
File "autofill_eval_spec.py", line 220, in validate_and_fill_leg
rclist.append(get_route_from_relation(t["relation"]))
File "autofill_eval_spec.py", line 156, in get_route_from_relation
return get_coords_for_relation(r["relation_id"],
File "autofill_eval_spec.py", line 149, in get_coords_for_relation
start_index = on_list.index(start_node)
ValueError: 6410508153 is not in listafter fixing the above as such: rclist = []
if "polyline" in t:
route_coords = get_route_from_polyline(t)
rclist.append(route_coords)
elif "relation" in t:
if isinstance(t["relation"], list):
for r in t["relation"]:
rclist.append(get_route_from_relation(r))
else:
rclist.append(get_route_from_relation(t["relation"]))
else:
# We need to find a point within the polygon to pass to the routing engine
start_coords_shp = geo.Polygon(start_polygon["geometry"]["coordinates"][0]).representative_point()
start_coords = geo.mapping(start_coords_shp)["coordinates"]
end_coords_shp = geo.Polygon(end_polygon["geometry"]["coordinates"][0]).representative_point()
end_coords = geo.mapping(end_coords_shp)["coordinates"]
print("Representative_coords: start = %s, end = %s" % (start_coords, end_coords))
route_coords = get_route_from_osrm(t, start_coords, end_coords)
rclist.append(route_coords)Is this on the right track? |
|
@singhish at a high level, I wonder if we should just make the decision to skip the relations and go to polylines and manually specified polygons right now. It looks like that kind of lookup is increasingly brittle. To debug your error further: we expect it to be in the list of nodes associated with the relation. it apparently is not. |
leads us to Checking that relation in OSM today, we find which does not go through node https://www.openstreetmap.org/node/6410508153 The Bear Transit Shuttle bus has been re-routed to go through the east fork on Shattuck instead of the west fork. |
|
@singhish I think that for the immediate term, we should focus on getting the berkeley reroutes into one filled spec, even if you have to merge them manually. Concretely, we want to the final filled spec to have:
I thought that modifying the input spec and the filled spec creation script would be easier than manually copy/pasting lines across files. But if it looks like this is dragging on, maybe we should just manually fix the filled spec and move on? How close do you think we are? |
|
After adding support for Addison and Shattuck, the script broke due to the "Only simple polygons supported" error. As a temporary fix, I'm enclosing any coordinates are flagged as more than length one within a list, and I'm getting plots produced that way. Why are some |
|
that might be an issue with the input, let me take a look. I didn't investigate further when I saw the error since I wasn't sure if the error was with the spec or with the script. |
I suspect the issues with the geometry verification have to do with the ill formatting that's going on with the nesting in the input spec. We can discuss this as well tomorrow. |
| (l["mode"], l["start_loc"]["properties"]["name"]), | ||
| "loc": _add_temporal_ground_truth(l["start_loc"], default_start_fmt_date, default_end_fmt_date) | ||
| }) | ||
| if isinstance(l["start_loc"], list): |
There was a problem hiding this comment.
This fixes the specific case of Addison and Shattuck. Does it fix all such similar cases? Can you test/walk through the use cases?
The points should be in an array, not in as separate points which look like multiple polygons
Fixed in the spec (singhish#8)
This has nothing to do with the geometry verification. It is because of the nested if for the start and end loc entries. You are effectively checking that the trajectories are between all pairs of start and end locations. Which is true if there is one start and one end location but breaks otherwise. |
|
The
|
| ] | ||
| }, | ||
| { | ||
| "id": "wait_for_city_bus_short_0", |
| ] | ||
| }, | ||
| { | ||
| "id": "wait_for_city_bus_short_1", |
|
Also for After adding an assert It turns out that is indeed the case |
|
for the overlapping time validation, can you add a small sample spec with an overlapping time range showing that it works? It looks right, but I don't know that it works without seeing that it can correctly detect an overlap. |
|
yes that ended up doing the trick. realized that it is the route that has to satisfy the start/end locations, not vice versa, as it the route that is being changed between dates. |
|
#48 (comment) is not yet fixed - I just double checked. Unfortunately, I cannot wait any more to merge this because I'm improving the evaluation sample for e-mission/e-mission-docs#624 and it needs to use the new format for the filled spec. So I am merging this and creating a new issue for the pending problem. |
|
Created new issue MobilityNet/mobilitynet.github.io#20 |
timespankey to handle information regarding the timespans of reroutes (will talk to you @shankari during our meeting tomorrow to figure out where to grab this information -- not sure if this is already encoded in thestart_fmt_dateandend_fmt_datekeys?)final_sfbayarea_filled_reroutesfor updated data containing reroute information