-
Notifications
You must be signed in to change notification settings - Fork 23
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
Update generic metrics for Average Miles for each mode (selected by u… #113
Conversation
…sers) chart, and update generic metrics sensed notebook for Average miles per transport mode selected (primary_mode).
…, made the timestamp more visible.
Slight modification to the above changes to incorporate the following:
Summary of overall changes:
|
I am not sure that this is the correct solution. Basically, if you have a metric that is showing up as blank, you have two options:
This implements (1). However, we already have an average trip length metric (the one with > 3 entries per mode). I don't see the need for a second one. For the record, we had the unfiltered metric implemented before, but ran into issues in which we had one scootershare trip that was 10 miles (potentially mislabeled) and it threw off the entire y axis. |
Summary of additional changes:
Execution:
Result:
|
viz_scripts/plots.py
Outdated
def barplot_sensed_mode(data,x,y,plot_title, labels, file_name): | ||
colours = dict(zip(labels, plt.cm.tab20.colors[:len(labels)])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this function? This is identical to barplot_mode
. Why not just use barplot_mode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did realize the function is identical to barplot_mode
. But I decided to keep an exclusively different function for the sensed
chart generation, despite being identical because:
- Following the existing pattern of function definition: The other existing function definition for
pie_chart_mode
is also identical to thepie_chart_sensed_mode
function. This led me to think, if we need to make changes exclusively impacting thesensed
charts, and not thenon-sensed
charts; having two distinct functions would be good decision.
This function barplot_sensed_mode
can be modifiable for changes related with sensed
charts, without having any impact for the non-sensed
charts generated in other notebooks.
I understand having two identical functions barplot_sensed_mode
& barplot_mode
brings up code-duplication. But I see a trade-off for the approach of having two functions will help us leverage easy modification in future.
Please let me know what would be the right approach in this case.
I think, if there is a probability of having different ways in which sensed
chart and non-sensed
charts would be generated, it seems like a good trade-off to keep two identical functions. But if these two charts generation would always be identical, I think using a single function is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on our discussion, I feel it's ideal to remove code-duplication and not engineer for future.
I have removed the redundant barplot_sensed_mode()
, and replaced the call with barplot_mode()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documenting the discussion here for the future.
But I see a trade-off for the approach of having two functions will help us leverage easy modification in future.
But if these two charts generation would always be identical, I think using a single function is a good idea.
I do not see how this will leverage easy modification in the future. If we do need to modify in the future, we will still need to test the modified implementation, whether we generate the new implementation by copying the existing implementation at that time or now. Once we know what the potential changes are, we may also choose to implement the changes by refactoring the current implementation instead of copying, it depends on the changes required. This is pretty much the textbook definition of DRY and over-engineering, and should be removed now.
Test scenario:
Executed the below script to run
Result:
The above result shows the chart being generated for |
viz_scripts/plots.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous changes. This entire file does not have any real changes, and should not appear in the PR. It's commit history should not be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reverted all the extraneous changes in this file. The file doesn't show up under "Files Changed.".
I tried to execute git checkout <commit_before_this_PR_change> -- viz_scripts/plots.py
, but this didn't show any staging file.
I am not sure if the commit history would be affected with this, though.
Squash-merging to avoid messing up the commit history |
…sers) chart, and update generic metrics sensed notebook for Average miles per transport mode selected (primary_mode).