search

insight-lane / crash-model

Build a crash prediction modeling application that leverages multiple data sources to generate a set of dynamic predictions we can use to identify potential trouble spots and direct timely safety interventions.
https://insightlane.org
MIT License
112 stars 40 forks source link

Aesthetic improvements to viz #229

Closed alicefeng closed 5 years ago

alicefeng commented 5 years ago

Finally merging in long-awaited improvements to the crash layer in the viz:

j-t-t commented 5 years ago

Can you talk in a little more detail about what you're trying to do in standardize_crashes? It sort of looked like the main goal was to output your new crash rollup file. Is there a purpose to taking out those blocks of code

It looks like you took datadir and city out as arguments to read_standardized_fields because they weren't used in that function. You'll need to update all calls to read_standardized_fields in src/data_standardization/tests/test_standardize_crashes.py to also remove those arguments.

With regard to your changes to writing the file, I'd move all of what you're adding into its own function, instead of part of it in main and part of it in its own function. It will be easier to write a test that way. Also, please add a description at the top of your new function inside of triple quotes, that says what it does and also gives information about what the arguments are to the function.

alicefeng commented 5 years ago

I'm not sure what happened with the deleted chunks of code. I did not remove anything from the script, only added to it so something weird happened there. At any rate, I think I've restored all of the deleted lines. I've also moved all of the code for rolling up the crashes into a function except for the part where the geojson file actually gets written.

Let me know if there are any other edits needed.

alicefeng commented 5 years ago

Added a test for make_crash_rollup. I used assert_frame_equal from pandas' testing module to test if the two dataframes were the same. Let me know if it'll cause any problems to use that (it looks like my test passed).

codecov[bot] commented 5 years ago

Codecov Report

Merging #229 into master will increase coverage by 0.2%. The diff coverage is 80.76%.

@@            Coverage Diff            @@
##           master     #229     +/-   ##
=========================================
+ Coverage   54.63%   54.83%   +0.2%     
=========================================
  Files          34       34             
  Lines        3324     3350     +26     
=========================================
+ Hits         1816     1837     +21     
- Misses       1508     1513      +5
j-t-t commented 5 years ago

Looks great!

terryf82 commented 5 years ago

@alicefeng is this ready for deployment to the showcase? If so, do we want to try and get https://github.com/Data4Democracy/crash-model/pull/218 merged in at the same time?

alicefeng commented 5 years ago

@terryf82 Yes, this can be pushed to the showcase! I agree about merging #218 in along with it.