Update clustering.py

[humbleOldSage]

Changes in clustering.py file to shift dependency from hlu09's tour_model_extended to main branch's trip_model. Still need to change type of data being passed to fit function for this to work. Marked with a TODO. Explained in detail at #35 (comment)

[shankari]

Much better....

Have you run the code?
Please indicate "testing done".
Do you get the same graphs as the paper?
Note that the check is not "do I get a graph", it is "do I get the same graph"

[humbleOldSage]

Have you run the code?

Yes

Do you get the same graphs as the paper?

Yet to confirm

Please indicate "testing done".

Ongoing. The way I am planning to test this is I'll match and compare labels generated by both custom branch and master branch. This will verify that master branch and custom branch are functioning similarly.

Is there any other way I can test this ?

[humbleOldSage]

Do you get the same graphs as the paper?

They differ. Let me check why this is happening.

[humbleOldSage]

Tested. This is running with no errors.
Can confirm this generates the same results.

[shankari]

this particular change seems fine if it works.

• Turned out to be pretty simple after all?!
• I would like to see more information in the PR issue that it works (screenshots, information about the model indicating that it works)
• is this the only notebook that is affected by the change? I know that we have a notebook which generates the performance (accuracy/F-score) of various algorithms. I would expect that it would also need to be changed...

[humbleOldSage]

• is this the only notebook that is affected by the change? I know that we have a notebook which generates the performance (accuracy/F-score) of various algorithms. I would expect that it would also need to be changed...

Almost All the other notebooks have dependencies on this module

[humbleOldSage]

• I would like to see more information in the PR issue that it works (screenshots, information about the model indicating that it works)

Screenshot from the latest run, so no errors.

[humbleOldSage]

Left is current result. Right is from research paper.
Suburban 50m.

Suburban 100m

Suburban 150m

[humbleOldSage]

Left is current result. Right is from research paper.
College 50m.

College 100m

College 150m

[humbleOldSage]

Since this is partially tested, I'll keep the PR as draft, as soon as I have completed the final testing, I'll mark it as ready to merge.

[shankari]

Almost done, just a few minor changes

[humbleOldSage]

Not tested. Needs Testing.

[shankari]

Even smaller cleanups

[humbleOldSage]

generate_figs_from_poster.ipynb :

plot after latest testing

snap from the research paper :

plot after latest testing

snaps from the research paper :

[humbleOldSage]

generate_figs_for_poster.ipynb :

On the left are Plots after current testing, on the right are images from runs of notebook with @hlu109 custom branch :

naive fixed-width clustering from the first user's data

150m

50m

100m

DBSCAN without SVM: home cluster with a blue cluster to the south that was merged in

DBSCAN + SVM: home cluster and blue cluster to the south have been separated

[humbleOldSage]

Clustering_example.ipynb

Left is current test result. Right is from research paper.
Suburban 50m.

Suburban 100m

Suburban 150m

Left is current result. Right is from research paper.
College 50m.

College 100m

College 150m

[humbleOldSage]

SVM_decision_boundary.ipynb :

On the left are plots from current test, on the right are plots from old runs :

[humbleOldSage]

get_cluster_performance.ipynb :

For each pair, top one is the result of current test, bottom one is result from older runs :

[humbleOldSage]

All model results :

[shankari]

@humbleOldSage I don't think you have addressed several of the prior review comments. They are fairly simple, so you might have just missed them - please check the review history carefully.

Please make sure that all comments are addressed before marking as ready for review.

[shankari]

Now that we are no longer on a custom branch, can't the distance calculation in e-mission-server be re-used here? Why do we need copy-pasted code? I am generally again copy-pasting to support DRY. Since this change has already had multiple revisions, I am OK with deferring this to the next PR, but I want to make sure that it is not forgotten.

[humbleOldSage]

We are indeed using e-mission-server.

Line 416:

  dist= ecc.calDistance([pta_lon,pta_lat],[ptb_lon,ptb_lat])     

ecc here is on e-mission-server :

  import emission.core.common as ecc

That's an old comment from Hannah that we should remove.

[humbleOldSage]

While predicting in the greedy_similarity_binning.py on e-mission-server, the flow goes like :

predict -> _nearest_bin ->similar ( in e-mission-server/emission/analysis/modelling/similarity/similarity_metric.py) ->similarity ( in e-mission-server/emission/analysis/modelling/similarity/od_similarity.py) -> ecc.calDistance.

Ans so currently we are using this ecc.calDistance

[shankari]

I think that you have misunderstood the comment. The comment is not about ecc.calDistance - it is clear that calDistance is from
This says that _distanceHelper is copied from e-mission-server, which is it

$ grep -r distance_helper emission/
emission//analysis/modelling/tour_model_first_only_orig/similarity.py:            if not self.distance_helper(a, b):
emission//analysis/modelling/tour_model_first_only_orig/similarity.py:    def distance_helper(self, a, b):
emission//analysis/modelling/tour_model/similarity.py:            if not self.distance_helper(a, b):
emission//analysis/modelling/tour_model/similarity.py:    def distance_helper(self, a, b):
emission//incomplete_tests/TestSimilarity.py:                    self.assertTrue(sim.distance_helper(b,c))

However, the implementation does seem to be a bit different, and I don't see a function with this name in trip_model. There must be an equivalent in trip_model to calculate distances between trips, which we should reuse here.

[humbleOldSage]

Other than ecc.calDistance, there just pre-processing in _distance_helper in the form of coordinate extraction :

        #tripa is taken from the test datframe. 
        #tripb is taken from the stored bin list.
        
        pta_lat = tripa[[loc_type + '_lat']]
        pta_lon = tripa[[loc_type + '_lon']]
        if loc_type == 'start':
            ptb_lat = tripb[1]
            ptb_lon = tripb[0]
        elif loc_type == 'end':
            ptb_lat = tripb[3]
            ptb_lon = tripb[2]

We do have extract_features function ( at e-mission-server/emission/analysis/modelling/similarity/od_similarity.py) on e-mission-server that extracts latitude and longitude of trips, but it works just for Entry type data ( since data frames are not used in e-mission-server).

There must be an equivalent in trip_model to calculate distances between trips, which we should reuse here.

For the reason above, there isn't. However, this is what we can do to use the _nearest_bin function ( e-mission-server/emission/analysis/modelling/trip_model/greedy_similarity_binning.py) which is closest to _distance_helper :

1. convert tripa ( the test trip) to entry type data using df_row_to_entry ( in e-mission-server/emission/storage/timeseries/builtin_timeseries.py).
2. Pass this entry to the _nearest_bin function.

Let me know if this works, and I'll test this.

[shankari]

From #37 (comment)

Since this change has already had multiple revisions, I am OK with deferring this to the next PR, but I want to make sure that it is not forgotten.

I am fine with returning to this later. However, eventually, we should simplify the codebase to either use only dataframes, or use only entries, or, if we are going to support some level of mix and match, have the utility functions support both combinations.

Can you please file an issue for this so that we don't forget it?

[humbleOldSage]

filed here #39 .

[shankari]

@humbleOldSage two more comments.

[shankari]

Squash-merging since this is 21 commits for some fairly simple changes.
@humbleOldSage please account for this while making any future changes.