Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update greedy_similarity_binning.py #930

Closed
wants to merge 2 commits into from
Closed

Update greedy_similarity_binning.py #930

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61ab4b9
Update greedy_similarity_binning.py
humbleOldSage Aug 11, 2023
2b5b06a
Updated builtin_timeseries.py
humbleOldSage Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions emission/analysis/modelling/trip_model/greedy_similarity_binning.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ class label to apply:
self.is_incremental = config['incremental_evaluation']

self.bins: Dict[str, Dict] = {}
self.tripLabels=[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@humbleOldSage humbleOldSage Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@humbleOldSage humbleOldSage Aug 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I'll remove the extra data structure and move all the additional computations to clustering.py.



def fit(self, trips: List[ecwc.Confirmedtrip]):
Expand Down Expand Up @@ -184,9 +185,11 @@ def _assign_bins(self, trips: List[ecwc.Confirmedtrip]):
logging.debug(f"adding trip to bin {bin_id} with features {trip_features}")
self.bins[bin_id]['feature_rows'].append(trip_features)
self.bins[bin_id]['labels'].append(trip_labels)
self.tripLabels.append(bin_id)
else:
# create new bin
new_bin_id = str(len(self.bins))
self.tripLabels.append(new_bin_id)
new_bin_record = {
'feature_rows': [trip_features],
'labels': [trip_labels],
Expand Down