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.

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

[coalesce] Use mean of grouped edge weights rather than minimum #127

Merged
merged 8 commits into from Feb 23, 2019

Conversation

yiyange
Copy link
Collaborator

@yiyange yiyange commented Feb 5, 2019

Using minimum of grouped edge weights effectively reduce path costs, and increase accessibility significantly.
The following map shows shortest path lengths from one single source to all destinations in Berkeley.
It changes from using original full network to coalesced network using min grouped edge weights. Noticing the shortest path lengths reduce significantly (darker color to lighter color)
use_min

The following map shows the same change but using coalesced network using mean (different color bins though). The change is less significant.
use_mean

Also some stats comparison:

Stats Full Network Coalesced w Min Coalesced w Mean
Max shortest path length 5172 3092 3493
Mean shortest path length 2488 1383 1619

Visuals made by UrbanFootprint :))

@kuanb
Copy link
Owner

kuanb commented Feb 5, 2019

Excellent observation - thank you!!

This also helps in working towards addressing various risks associated with this issue: #126

@yiyange yiyange changed the title [coalesce] Use mean of grouped edge weights rather than minimum [coalesce] Use mean of grouped edge weights rather than minimum - WIP Feb 6, 2019
@kuanb kuanb changed the title [coalesce] Use mean of grouped edge weights rather than minimum - WIP [coalesce] Use mean of grouped edge weights rather than minimum Feb 6, 2019
@kuanb
Copy link
Owner

kuanb commented Feb 6, 2019

TODO:

  • Update tests
  • Explore impacts of performing with max instead (be conservative)

@yiyange
Copy link
Collaborator Author

yiyange commented Feb 6, 2019

@kuanb i updated the logic and reran, the impact is much less unfortunately. but it is a start.

if not G.has_edge(rn1, rn2):
# Append new edge with attributes to list
length = avg_length
# Note: the mode is determined by the first edge
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kuanb what's your thought on this?

Copy link
Owner

Choose a reason for hiding this comment

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

I think coalesce is sort of a mess with transit... Let's figure it out for walk first... I think assume a comment like you did is good. Maybe add a todo. In future we should segment coalesce by mode

peartree/toolkit.py Outdated Show resolved Hide resolved
Copy link
Owner

@kuanb kuanb left a comment

Choose a reason for hiding this comment

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

Thanks will circle back tomorrow morning


# Second step; which uses results from edge_df grouping/parsing
edges_to_add = []
# Go through each edge
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: Extraneous comment, would remove

if not G.has_edge(rn1, rn2):
# Append new edge with attributes to list
length = avg_length
# Note: the mode is determined by the first edge
Copy link
Owner

Choose a reason for hiding this comment

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

I think coalesce is sort of a mess with transit... Let's figure it out for walk first... I think assume a comment like you did is good. Maybe add a todo. In future we should segment coalesce by mode

peartree/toolkit.py Outdated Show resolved Hide resolved
@kuanb
Copy link
Owner

kuanb commented Feb 23, 2019

@yiyange Update on this PR: I just updated tests, tweaked logic. Should be passing the Travis tests shortly and will merge in after that - thanks again.

Copy link
Owner

@kuanb kuanb left a comment

Choose a reason for hiding this comment

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

Updated tests, modified and cleaned up logic that was being adjusted. Added some TODO flags for a deeper dive in the future.

@codecov
Copy link

codecov bot commented Feb 23, 2019

Codecov Report

Merging #127 into master will increase coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
+ Coverage   91.43%   91.68%   +0.24%     
==========================================
  Files          13       13              
  Lines        1109     1106       -3     
==========================================
  Hits         1014     1014              
+ Misses         95       92       -3
Impacted Files Coverage Δ
peartree/toolkit.py 93.5% <100%> (+1.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 761910a...09a5732. Read the comment docs.

@kuanb kuanb merged commit 57c6df3 into master Feb 23, 2019
@kuanb kuanb deleted the yiyan-test-new-edge-weight branch February 23, 2019 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants