2357 add junctions to suburban widgets #2476

ziv17 · 2023-09-15T11:08:31Z

The previous PR was closed by mistake. Opening a new one.

atalyaalon · 2023-09-27T14:38:36Z

@ziv17 Following the correspondence here you can find a road segment table (As to 03-2022) here.

I waiting for a new updated of that table, hope to get it soon. For now you can make sure to create a xlsx and adjust our parser code AND make sure loading works as expected. Let me know if you find additional errors as the one you found and I'll send the new segments table ASAP

atalyaalon · 2023-09-27T16:27:18Z

Also @ziv17 can we create tests that use GPS data of junctions (we have this data from the CBS) and use segments GPS and maybe even data from google maps Geocoding to find errors?

Editing: this is for next prs, I suggest we first insert your current prs so the cache data will be updated with the junctions in the segment

codecov-commenter · 2023-12-02T10:03:47Z

Codecov Report

Attention: 82 lines in your changes are missing coverage. Please review.

Comparison is base (30003a4) 53.16% compared to head (04a39c9) 53.03%.

Files	Patch %	Lines
anyway/parsers/suburban_junctions.py	0.00%	74 Missing ⚠️
main.py	0.00%	5 Missing ⚠️
anyway/models.py	87.50%	1 Missing ⚠️
anyway/request_params.py	83.33%	1 Missing ⚠️
...tions_widgets/accident_count_by_severity_widget.py	50.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #2476      +/-   ##
==========================================
- Coverage   53.16%   53.03%   -0.14%     
==========================================
  Files         117      119       +2     
  Lines        9670     9788     +118     
==========================================
+ Hits         5141     5191      +50     
- Misses       4529     4597      +68

Flag	Coverage Δ
unittests	`53.03% <55.67%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ziv17 · 2023-12-11T19:45:18Z

Hi @atalyaalon ,
I am not sure how to handle discrepancies like the following:
Accidents in suburban junction 4193 (road=1, מחלף דניאל) report it is on km 22 (see query below)
Road segment 10054 starts at km: 22.9
I did not find the road segment that ends there.

query:
select road1, non_urban_intersection , km, km_accurate, accident_timestamp from markers_hebrew where non_urban_intersection=4193 order by accident_timestamp

ziv17 · 2023-12-16T09:42:47Z

Hi @atalyaalon ,
I am not sure how to handle discrepancies like the following:
Accidents in suburban junction 4193 (road=1, מחלף דניאל) report it is on km 22 (see query below)
Road segment 10054 starts at km: 22.9
I did not find the road segment that ends there.

query:
select road1, non_urban_intersection , km, km_accurate, accident_timestamp from markers_hebrew where non_urban_intersection=4193 order by accident_timestamp

ziv17 · 2023-12-16T09:45:05Z

Hi @atalyaalon
In essence, this PR adds OR suburban_junction in ( <juncltion list> ) to widget queries that has the segment_id field in the filter. It is finally ready.

atalyaalon · 2023-12-20T17:26:03Z

@ziv17 , this pr looks great! Awesome work!
I want to merge ASAP.
A few questions right before merging:

I assume once merged we should load all of the CBS Data from past years to make sure the RoadJunctionKM tables are filled?
Should we add non_urban_intersection index to relevant tables that use get_query (say markers_hebrew) to make sure cache fill runs in a reasonable time?
Besides get_query, do we have other calls in our code of infographics widgets we need to adjust? (We can open separate issues for them and merge this one)
TODOs in code:

TODO of implementation of top_road_segments_accidents_per_km_widget - perhaps you can open a separate issue?
TODO of remove road_segment_name if road_segment_id exists: I saw you have a draft pr. I assume it would be the next pr we'll merge?

ziv17 · 2023-12-20T18:39:20Z

Hi @atalyaalon ,

1. I assume once merged we should load all of the CBS Data from past years to make sure the RoadJunctionKM tables are filled?

Yes, correct

2. Should we add non_urban_intersection index to relevant tables that use get_query (say markers_hebrew) to make sure cache fill runs in a reasonable time?

Yes

3. Besides get_query, do we have other calls in our code of infographics widgets we need to adjust? (We can open separate issues for them and merge this one)

If there are widgets that do not use get_query(), then yes. I can take it as a task if you want.

4. TODOs in code:


* TODO of implementation of top_road_segments_accidents_per_km_widget - perhaps you can open a separate issue?

Improve implementation: db filter of string not equal and included in list #2507

* TODO of remove road_segment_name if road_segment_id exists: I saw you have a draft pr. I assume it would be the next pr we'll merge?

Remove road_segment_name from query if road_segment_id is present. [#2503]

ziv17 · 2023-12-20T19:06:08Z

Hi @atalyaalon ,
I incorporated the latest commit in dev, and the tests fail on

def parse_html_walla(item_rss, html_soup):
    # For some reason there's html here
    description = BeautifulSoup(item_rss["summary"], features="lxml").text

    author = html_soup.find("div", class_="author").get_text().strip()
    return author, description

in line author = html_soup.find("div", class_="author").get_text().strip()
The find returns None, and the get_text() fails.
(This code is 2.5 years old). This test does not provide much value, as it only tests that the real data that returns in not empty.
What do you think?

atalyaalon · 2023-12-20T19:54:05Z

Hi @atalyaalon , I incorporated the latest commit in dev, and the tests fail on
def parse_html_walla(item_rss, html_soup):
    # For some reason there's html here
    description = BeautifulSoup(item_rss["summary"], features="lxml").text

    author = html_soup.find("div", class_="author").get_text().strip()
    return author, description
in line author = html_soup.find("div", class_="author").get_text().strip() The find returns None, and the get_text() fails. (This code is 2.5 years old). This test does not provide much value, as it only tests that the real data that returns in not empty. What do you think?

@ziv17 In that case you can remove the test

atalyaalon · 2023-12-20T20:07:36Z

Hi @ziv17 , awesome work again :) answers below:

Hi @atalyaalon ,

1. I assume once merged we should load all of the CBS Data from past years to make sure the RoadJunctionKM tables are filled?

Yes, correct

Great

Should we add non_urban_intersection index to relevant tables that use get_query (say markers_hebrew) to make sure cache fill runs in a reasonable time?
Yes

Great, can you create a list of the tables and create new migration / add to current DB migration in this pr?
I want to make sure it's added in this PR

Besides get_query, do we have other calls in our code of infographics widgets we need to adjust? (We can open separate issues for them and merge this one)
If there are widgets that do not use get_query(), then yes. I can take it as a task if you want.

Yes, can you create a list of these widgets in an issue and we'll priorities? (This won't impact current PR, we won't wait)

TODOs in code:

TODO of implementation of top_road_segments_accidents_per_km_widget - perhaps you can open a separate issue?
Improve implementation: db filter of string not equal and included in list #2507
* TODO of remove road_segment_name if road_segment_id exists: I saw you have a draft pr. I assume it would be the next pr we'll merge?
Remove road_segment_name from query if road_segment_id is present. [#2503]

ziv17 · 2023-12-20T20:22:26Z

Yes, can you create a list of these widgets in an issue and we'll priorities? (This won't impact current PR, we won't wait)

Opened 2508

ziv17 · 2023-12-20T20:24:29Z

can you create a list of the tables and create new migration / add to current DB migration in this pr?
I want to make sure it's added in this PR

Will do

ziv17 · 2023-12-20T20:49:23Z

Hi @atalyaalon ,
in some widgets (e.g. anyway/widgets/road_segment_widgets/accident_type_vehicle_type_road_comparison_widget.py) there is a query with road (1 or 2), but no road_segment. Should all junctions on these roads be added too?

ziv17 · 2023-12-20T21:12:40Z

Below widgets do not use get_query(), and need to be handled specifically.
anyway/widgets/widget_utils.py, get_involved_counts)
anyway/widgets/road_segment_widgets/killed_and_injured_count_per_age_group_widget_utils.py

anyway/widgets/all_locations_widgets/seriously_injured_killed_in_bicycles_scooter_widget.py
anyway/widgets/road_segment_widgets/front_to_side_accidents_by_severity.py
anyway/widgets/road_segment_widgets/killed_and_injured_count_per_age_group_stacked_widget.py
anyway/widgets/road_segment_widgets/killed_and_injured_count_per_age_group_widget.py
anyway/widgets/road_segment_widgets/road2_plus1_widget.py: use road_segment_id instead of _name.

ziv17 · 2023-12-21T21:14:28Z

Below widgets do not use get_query(), and need to be handled specifically.
anyway/widgets/widget_utils.py, get_involved_counts)
anyway/widgets/road_segment_widgets/killed_and_injured_count_per_age_group_widget_utils.py

anyway/widgets/all_locations_widgets/seriously_injured_killed_in_bicycles_scooter_widget.py
anyway/widgets/road_segment_widgets/front_to_side_accidents_by_severity.py
anyway/widgets/road_segment_widgets/killed_and_injured_count_per_age_group_stacked_widget.py
anyway/widgets/road_segment_widgets/killed_and_injured_count_per_age_group_widget.py
anyway/widgets/road_segment_widgets/road2_plus1_widget.py: use road_segment_id instead of _name.

Closes #2508

ziv17 · 2023-12-21T21:33:16Z

Tables with road_segment_id or non_urban_intersection:
AccidentMarkerView - markers_hebrew -road_segment_id:yes non_urban_intersection:no
InvolvedMarkerView - involved_markers_hebrew - road_segment_id:yes, non_urban_intersection:no
VehicleMarkerView - vehicles_markers_hebrew - road_segment_id:yes, non_urban_intersection:no

Adding non_urban_intersection index to above table.

ziv17 · 2023-12-22T08:42:23Z

Hi @atalyaalon
I added the junctions to widgets that I missed, and added non_urban_intersection index to markers_hebrew, involved_markers_hebrew, and vehicles_markers_hebrew

ziv17 · 2023-12-23T09:04:28Z

Hi @atalyaalon This PR is ready for your review

atalyaalon · 2023-12-31T17:35:39Z

Note: No need to resolve conflicts, I resolved them here: #2527
I'll go over this pr during this week, perform some tests and hopefully we'll merge it in the coming days, well done Ziv.

ziv17 · 2024-01-12T13:33:49Z

Closes #2546

ziv17 · 2024-01-12T13:38:44Z

Closes #2546

Adding SegmentJunctions class. Adding non_urban_intersection index to markers_hebrew involved_markers_hebrew vehicles_markers_hebrew Remove test_scrape_sanity_online_walla() as it depends on actual values returned from Walla, and is not stable

Adding suburban_junctions.py and a command process suburban-junctions to load SuburbanJunction and RoadJunctionKM db tables from added file in prev commit. Removed updating these tables from accidents. Remove trailing blank from hebrew name of junction 7039 in suburban_junctions.xlsx

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from 7a6324a to 2a315c2 Compare October 3, 2023 16:30

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from 2a315c2 to a3c91e8 Compare December 2, 2023 08:47

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from f422db9 to aaf2a78 Compare December 9, 2023 16:50

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from abd3345 to 0cdc9a5 Compare December 16, 2023 08:49

ziv17 requested a review from atalyaalon December 16, 2023 09:37

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from 0cdc9a5 to 6b29497 Compare December 20, 2023 18:25

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from b59a08c to 87c5698 Compare December 22, 2023 08:55

atalyaalon mentioned this pull request Dec 31, 2023

merge Pull 2476 to dev #2527

Closed

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from 87c5698 to 1ac258e Compare January 12, 2024 06:15

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from 5a3e5a8 to bd935a1 Compare January 12, 2024 13:38

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch 3 times, most recently from cf971f6 to 439a8fe Compare January 15, 2024 14:07

ziv17 added 3 commits January 15, 2024 19:12

Adding table road_junction_km.

f89413e

Adding SegmentJunctions class. Adding non_urban_intersection index to markers_hebrew involved_markers_hebrew vehicles_markers_hebrew Remove test_scrape_sanity_online_walla() as it depends on actual values returned from Walla, and is not stable

Adding suburban_junctions.xlsx file from cbs dated 21-Dec-2023

a6d9789

ziv17 force-pushed the 2357-add-junctions-to-suburban-widgets branch from 4689382 to 04a39c9 Compare January 15, 2024 18:33

atalyaalon merged commit 415e749 into data-for-change:dev Jan 17, 2024
3 checks passed

ziv17 deleted the 2357-add-junctions-to-suburban-widgets branch January 26, 2024 08:19

ziv17 mentioned this pull request Feb 9, 2024

List widgets that do not use get_query() function, and implement 2357 for them. #2508

Closed

This was referenced May 9, 2024

Read suburban junctions file from CBS to our SuburbanJuncion table #2546

Closed

Suburban Widgets - Modify calculations #2357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2357 add junctions to suburban widgets #2476

2357 add junctions to suburban widgets #2476

ziv17 commented Sep 15, 2023

atalyaalon commented Sep 27, 2023 •

edited

Loading

atalyaalon commented Sep 27, 2023 •

edited

Loading

codecov-commenter commented Dec 2, 2023 •

edited

Loading

ziv17 commented Dec 11, 2023

ziv17 commented Dec 16, 2023

ziv17 commented Dec 16, 2023

atalyaalon commented Dec 20, 2023

ziv17 commented Dec 20, 2023 •

edited

Loading

ziv17 commented Dec 20, 2023

atalyaalon commented Dec 20, 2023

atalyaalon commented Dec 20, 2023 •

edited

Loading

ziv17 commented Dec 20, 2023

ziv17 commented Dec 20, 2023

ziv17 commented Dec 20, 2023 •

edited

Loading

ziv17 commented Dec 20, 2023

ziv17 commented Dec 21, 2023 •

edited

Loading

ziv17 commented Dec 21, 2023 •

edited

Loading

ziv17 commented Dec 22, 2023

ziv17 commented Dec 23, 2023

atalyaalon commented Dec 31, 2023

ziv17 commented Jan 12, 2024

ziv17 commented Jan 12, 2024

2357 add junctions to suburban widgets #2476

2357 add junctions to suburban widgets #2476

Conversation

ziv17 commented Sep 15, 2023

atalyaalon commented Sep 27, 2023 • edited Loading

atalyaalon commented Sep 27, 2023 • edited Loading

codecov-commenter commented Dec 2, 2023 • edited Loading

Codecov Report

ziv17 commented Dec 11, 2023

ziv17 commented Dec 16, 2023

ziv17 commented Dec 16, 2023

atalyaalon commented Dec 20, 2023

ziv17 commented Dec 20, 2023 • edited Loading

ziv17 commented Dec 20, 2023

atalyaalon commented Dec 20, 2023

atalyaalon commented Dec 20, 2023 • edited Loading

ziv17 commented Dec 20, 2023

ziv17 commented Dec 20, 2023

ziv17 commented Dec 20, 2023 • edited Loading

ziv17 commented Dec 20, 2023

ziv17 commented Dec 21, 2023 • edited Loading

ziv17 commented Dec 21, 2023 • edited Loading

ziv17 commented Dec 22, 2023

ziv17 commented Dec 23, 2023

atalyaalon commented Dec 31, 2023

ziv17 commented Jan 12, 2024

ziv17 commented Jan 12, 2024

atalyaalon commented Sep 27, 2023 •

edited

Loading

atalyaalon commented Sep 27, 2023 •

edited

Loading

codecov-commenter commented Dec 2, 2023 •

edited

Loading

ziv17 commented Dec 20, 2023 •

edited

Loading

atalyaalon commented Dec 20, 2023 •

edited

Loading

ziv17 commented Dec 20, 2023 •

edited

Loading

ziv17 commented Dec 21, 2023 •

edited

Loading

ziv17 commented Dec 21, 2023 •

edited

Loading