Update greedy_similarity_binning.py #930
Conversation
Added a new list type variable to store labels at one place in order of trips processed.
| @@ -121,6 +121,7 @@ class label to apply: | |||
| self.is_incremental = config['incremental_evaluation'] | |||
|
|
|||
| self.bins: Dict[str, Dict] = {} | |||
| self.tripLabels=[] | |||
There was a problem hiding this comment.
I don't see why we need a separate array here.
the bins already are indexed by the bin_id, and bin has the labels in it
"25": {
'feature_rows': ....
'labels':...
}
So I don't see what this is adding to the data structure
There was a problem hiding this comment.
The clustering.py file ( which calls this function) file builds on data_loc dataframe that requires
Trip 1 --> bin No. (say 2)
Trip 2. --> bin No. (say 3)
Trip 3 --> bin No.( say 2)
Trip 4 --> bin No.( say 1)
Trip 5 --> bin No.( say 1)
.
.
.
However, the way they are stored here is .
"1": { 'feature_rows': [ [feature of Trip4], [feature of Trip5]]
'labels': ...
}
"2": { 'feature_rows': [ [feature of Trip1], [feature of Trip3]]
'labels': ...
}
"3": { 'feature_rows': [ [feature of Trip2]]
'labels': ...
}
Surely, we can extract the trip and its bin from the way they are already stored, but creating a separate array initially itself feels more efficient.
There was a problem hiding this comment.
But the problem with creating an additional data structure is that then we are changing production code to make analysis easier. We don't really need the trip -> bin mapping on production, and at some point, might want to include memoization as well. So I would prefer doing the additional work in clustering.py to create the dataframe from the output of the production model and not the other way around.
There was a problem hiding this comment.
That makes sense. I'll remove the extra data structure and move all the additional computations to clustering.py.
These changes were done to return `entry` type data ( alongside dataframe) to clustering_example.ipynb.
|
|
||
| entryList=[] | ||
|
|
There was a problem hiding this comment.
@humbleOldSage this is wrong. There is not only one set of entries in the database. Please read and understand the data model from chapter 5 of my thesis.
There was a problem hiding this comment.
Figured this was not necessary
| def getEntryList(self): | ||
| return self.entryList | ||
|
|
There was a problem hiding this comment.
again, this is wrong as well. you can't return the entry list because there is not just one.
There was a problem hiding this comment.
there are existing methods to get an entry list. the trip model uses existing methods. You should not have to make changes to the BuiltinTimeseries for this change.
| for e in entry_it: | ||
| BuiltinTimeSeries.entryList.append(map_fn(e)) | ||
| df = pd.DataFrame(BuiltinTimeSeries.entryList) | ||
|
|
|
NO changes needed here if, e-mission/e-mission-eval-private-data#37 are approved. |
|
Closing this since all changes are in e-mission/e-mission-eval-private-data#37 |
Added a new list type variable to store labels at one place in order of trips processed. Explained at e-mission/e-mission-eval-private-data#35 (comment) and e-mission/e-mission-eval-private-data#35 (comment).