Draft PR For Distance Confusion Matrices #71
Conversation
|
@rahulkulhalli can you update the PR after removing outputs, that will let me see the diff. |
|
@rahulkulhalli to remove outputs, you can use the "Restart kernel and clear outputs" option in a standard jupyter notebook. |
|
@shankari Outputs cleared. |
|
@shankari I've made some amends to the code. It generates the CMs for the trajectory data. Please review the code at your availability. |
There was a problem hiding this comment.
@rahulkulhalli High level looks fine. Suggested changes:
- remove FILE_MAPPING
- rename all
gt_variables toref_ - consider treating all runs the same way (don't special case 0)
- remove extraneous changes for easier review
- fix hardcoded replacement of
durwith dist`
Note that we had discussed generalizing this notebook instead of making a copy, or replacing values. So you could pass in a flag for the suffix, and have an associated function to compute the metric.
Once you refactor to pass in "dist" or "duration", it wouid be cool to compare them. Are the duration CMs fairly close to the distance CMs in terms of proportion/probability or are they different? It might also convert the CM into probabilities (divide by the sum?) so that they are both directly comparable without relying on the CM colors.
classification_analysis.ipynb
Outdated
| "FILE_MAPPING = {\n", | ||
| " pv_la: \"unimodal_trip_car_bike_mtv_la\",\n", | ||
| " pv_sj: \"car_scooter_brex_san_jose\",\n", | ||
| " # pv_ucb: \"train_bus_ebike_mtv_ucb\"\n", | ||
| "}" |
There was a problem hiding this comment.
I thought that this was temporary code so didn't comment on it earlier. you should be able to retrieve the timeline id directly from the pv by using pv.spec_details.spec_id
classification_analysis.ipynb
Outdated
| "source": [ | ||
| "from pathlib import Path\n", | ||
| "\n", | ||
| "def get_reference_trajectory(trip_id: str, section_id: str):\n", |
There was a problem hiding this comment.
if you are going to concatenate all the data for a single trip, then why do you need the sectionid?
There was a problem hiding this comment.
Thank you, that's a valid assessment. Initially, I aimed to design the function so as to be able to return a single data frame. I will make amends accordingly.
There was a problem hiding this comment.
@rahulkulhalli it's fine to leave as is as well - see my next comment below.
Just document why you chose the final design
classification_analysis.ipynb
Outdated
| " # now, we build a timeline for each trip\n", | ||
| " trip = tr.copy()\n", | ||
| " trip['ss_timeline'] = tr_ss\n", | ||
| " trip['gts_timeline'] = tr_gts\n", | ||
| " trip['gt_trajectory'] = pd.concat(gt_traj, axis=0).reset_index(drop=True, inplace=False)\n", | ||
| " \n", | ||
| " trips.append(trip)\n", |
There was a problem hiding this comment.
weren't you going to move this into the if loop?
classification_analysis.ipynb
Outdated
| " if filtered_gt_distance.shape[0] == 0:\n", | ||
| " dist = 0\n", |
There was a problem hiding this comment.
Does this happen a lot? I would not expect that to happen and we might want to assert instead
There was a problem hiding this comment.
It happens on some occasions, yes.
There was a problem hiding this comment.
@rahulkulhalli can we print out the trip/section for which this happens (in addition to setting dist=0)? I don't think this should happen, and I would like to verify against the stored reference trajectories for the specific use cases.
@rahulkulhalli There is a standard scikit-learn method to normalize confusion matrices that you can look at to do this 😄 |
|
@shankari Updated the notebook and added an entry to .gitignore. Amendments made:
The code runs until the plot_cm(). Please generate the CMs and verify whether they are as per expectations. TODO:
|
What normalization would you like me to incorporate? We could normalize w.r.t. the rows, the columns, or both. I can also add an extra kwarg in the plot function that would accept the desired normalization mode (None by default) |
classification_analysis.ipynb
Outdated
| @@ -708,7 +683,7 @@ | |||
| " trips = test_trip if type(test_trip) is list else [test_trip]\n", | |||
| " TP, FN, FP, TN = {}, {}, {}, {}\n", | |||
| " for trip in trips:\n", | |||
| " gt_trajectory = trip['gt_trajectory']\n", | |||
| " trajectory_data = trip['trajectory_data']\n", | |||
There was a problem hiding this comment.
I'm going to ask for changes again, but can we use the word reference or the prefix ref? that helps us distinguish between sensed trajectories and the reference trajectories.
|
@shankari, while running a sanity check against the notebook, I found this in the trajectoy data:
There seem to be some readings with |
|
@rahulkulhalli we should not filter the reference trajectory on android or ios. The reference trajectory is like derived ground truth. There is not a separate ground truth for android and for iOS. So there is not a separate reference trajectory for android and iOS. The sensed trajectories are android or iOS specific, the reference trajectories are not. For the record, 'midpoint' means that the reference trajectory for that section was the midpoint of the android and iOS accuracy control streams. |
|
iOS distance and duration confusion matrices [Top: Distance, Bottom: Duration] |
|
Android distance and duration confusion matrices [Top: Distance, Bottom: Duration] |
classification_analysis.ipynb
Outdated
| @@ -293,20 +278,26 @@ | |||
| "\n", | |||
| " tr_gts.append(gts)\n", | |||
| "\n", | |||
| " trajectory_data = get_reference_trajectory(pv.spec_details.CURR_SPEC_ID, tr['trip_id_base'])\n", | |||
| " trajectory_data = get_reference_trajectory(pv.spec_details.CURR_SPEC_ID, trip['trip_id_base'])\n", | |||
There was a problem hiding this comment.
| " trajectory_data = get_reference_trajectory(pv.spec_details.CURR_SPEC_ID, trip['trip_id_base'])\n", | |
| " curr_ref_trajectory = get_reference_trajectory(pv.spec_details.CURR_SPEC_ID, trip['trip_id_base'])\n", |
classification_analysis.ipynb
Outdated
| "\n", | ||
| " if normalization == 'pred':\n", | ||
| " # After transposing, predictions are axis=0\n", |
There was a problem hiding this comment.
| "\n", | |
| " if normalization == 'pred':\n", | |
| " # After transposing, predictions are axis=0\n", | |
| "\n", | |
| " # Copied from the sklearn implementation of confusion matrix.normalize\n", | |
| " ..... (add URL Here) \n", | |
| " if normalization == 'pred':\n", | |
| " # After transposing, predictions are axis=0\n", |
classification_analysis.ipynb
Outdated
| " ax[k].text(j, i, np.round(df.transpose().iat[i,j], 3), horizontalalignment='center', \n", | ||
| " color='white' \n", | ||
| " if df.transpose().iat[i,j] < color_thresh \n", | ||
| " else 'black')\n", | ||
| " \n", |
There was a problem hiding this comment.
what is the rationale for this change (e.g. adding the 3 parameter?)
There was a problem hiding this comment.
After normalizing, the calculated values are floating point values and do not round-off whilst displaying the confusion matrix. This makes the display texts overlap each other, rendering the entire confusion matrix unreadable. If normalization is not required, we cast to int (default implementation)
There was a problem hiding this comment.
I assumed that 3 digit precision would be an ideal trade-off between readability and precision.
classification_analysis.ipynb
Outdated
| "def export_confusion_matrix(matrix: pd.DataFrame, output_dir: Path):\n", | ||
| " pass" |
There was a problem hiding this comment.
haven't you implemented this yet?
There was a problem hiding this comment.
Yes, it is implemented. I will push the finished code.
|
These: are very sketchy. While the distance matrix might have a different proportion that the duration matrix, I find it hard to believe that there could be so many more zeros in distance versus duration. (as an aside, these should have "distance" and "duration" in the title to distinguish between them). I checked for it in the code review and I thought I saw it... ah it is only for I would focus on the selected matrix, and the entries that are obviously and blatantly different. 
I will try to do this too, but unfortunately, I have other priorities to work on as well 😄 |
|
or maybe even better is GIS, iOS, HAMFDC has a zero for distance 
Or HAHFDC, GIS, iOS By focusing on a single mode, we can expect the distance and duration to be proportional. If not, we may have a bug. |
|
Noted. I will focus on the recommended parameters and see if there is any bug in the code. |
|
@rahulkulhalli when did you last run the duration code? I wrote a new function to generate side by side distance and duration matrices and I am seeing a lot of zeros in the duration matrix as well. And of course, the notebook has been running only distance lately.
|
|
Note also that removing the normalization makes it seem better, at least for duration. |
|
Duration CM: From the paper (L), from the computation in the notebook (R), from the comparison (bottom): 
|
I ran it today itself. The generated plots are from today afternoon. |
I would want to understand this better before declaring victory. It is also super small but , but I am worried that it might be a symptom of something more serious that we are missing. |
|
I agree. I will investigate why the NO_SENSED_MIDDLE values are being missed. |
|
I tried to hack together a solution for the polygons for at least the start and end of travel legs by forcing the use of the "midpoint" method. The good news is that we do have a filled in reference trajectory. The bad news is that because we consider all the 
|
|
Investigation log no. 1: Confusion matrix invocation: Output:Majority NO_SENSED_MIDDLEs for gts['mode'] = BICYCLING |
|
Investigation log 2: Confusion matrix invocation: Output:Majority NO_SENSED_MIDDLEs for gts['mode'] = WALKING |
Right, so we are excluding all I really just wanted to understand the |
|
Log for UCB: Invocation parameters: Output:Majority NO_SENSED_MIDDLEs for gts['mode'] = WALKING |
|
Edited the logs to exclude NO_GT_MIDDLE |
so which timeline did this case happen in and were we in fact missing a reference trajectory there? |
|
This happened for the LA timeline. Indeed, we do not have any reference trajectory datapoints within the time range. |
|
Parameters: Column normalization: Row normalization: |
|
The column normalizations are what is more important, since we use them in the scripts for the downstream metrics. And fortunately, they seem pretty close! So this should not affect the results for the "count every trip" papers significantly, which means that we will likely not have to rewrite them. |
|
Tried to filter separately by start and end and it kind of worked but with some kinks 
Then I realized that this filters everything except start!! |
|
@rahulkulhalli the column normalizations seem broken - e.g. the bicycling column does not add up to 1. |
|
Filtering properly, everything seems to work 
But I have an error in the ct computationWith Fix that and then regenerate everything. |
|
Bingo! Top (tf), bottom (cf) |
|
@shankari Normalization is fixed. Adding plots here: Column normalization: Row normalization: |
|
Fixed. Next tried on one of the HOWEVER there are temporal points in the reference trajectory 
Let's see why this is broken. We can revert the zero check in that case. |
|
Ah it's because we were printing the wrong output ( Filtering at ends, for threshold 25, before merging, android: len(start_unfiltered_loc_a_df)=177, len(end_unfiltered_loc_a_df)=177 -> len(start_loc_df_a)=101, len(end_loc_df_a)=0, ios: len(start_unfiltered_loc_b_df)=129, len(end_unfiltered_loc_b_df)=129 -> len(start_loc_df_b)=93, len(end_loc_df_b)=13 I guess we can also have the other side. So we keep the zero checks, but if one of them is non-zero, we return a match to the ground truth using the code from |
|
Ok, so I actually implemented this But the problem is that, once we sort by timestamp, the iOS entries in the polygon are interspersed with the android locations outside the polygon
So we get something that looks like this 
we can probably do something fancy to fix this - like merging these entries and seeing which is closer to the ground truth but I'm not going to attempt it now. We are either outside the polygon or inside the polygon, and if two of our phones tell us inconsistent results - it is not clear which one to believe. We could also do something like delete the overlapping time windows for the end polygon from the trajectory, but I don't want to generalize from n=1 So if either of the trajectories is not present, we just return 0. |
|
@rahulkulhalli I have added computations of the reference trajectories in the start and end polygons as well. These expanded trajectories still have the limitation that, as The main pending limitation is that we skip reference trajectory creation if either of the accuracy control phones has no points inside the start or end polygons, which is reasonable to state and move on. The reference trajectories without the start and end polygons are in |
|
@shankari The stable code (without the inclusion of the start and end polygons) is pushed. |
|
CMs with ends included. iOS, HAHFDC, GIS, [gisv_la, gisv_ucb, gisv_sj]: android, HAMFDC, GIS, [gisv_la, gisv_ucb, gisv_sj]: (Both are column-normalized) |
|
I have added a flag |
|
Have you addressed all of my comments? If so, you should move this off draft. |
Questions: