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

1647 pedestrian city vs street accident widget #1726

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

rsperer
Copy link

@rsperer rsperer commented Feb 28, 2021

This wizard (as far as I can tell) is the first one implemented that requires an urban location (street rather than road and yishuv name).
I am assuming that the type of location affects which wizards are relevant. I added a boolean method to Wizard class is_urban and considered its relation to the newsflash location type in generate_widgets.
I then went ahead to implement the widget in accordance with its reddish specification and verified I got the same numbers.
closes #1647

@codecov
Copy link

codecov bot commented Feb 28, 2021

Codecov Report

Merging #1726 (0b09246) into dev (b6db369) will decrease coverage by 1.56%.
The diff coverage is 53.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1726      +/-   ##
==========================================
- Coverage   53.63%   52.07%   -1.57%     
==========================================
  Files          56       56              
  Lines        6272     6316      +44     
==========================================
- Hits         3364     3289      -75     
- Misses       2908     3027     +119     
Flag Coverage Δ
unittests 52.07% <53.84%> (-1.57%) ⬇️

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

Impacted Files Coverage Δ
anyway/infographics_utils.py 71.49% <53.84%> (-1.37%) ⬇️
anyway/parsers/location_extraction.py 24.74% <0.00%> (-46.91%) ⬇️
anyway/parsers/news_flash_db_adapter.py 60.86% <0.00%> (-8.70%) ⬇️

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 b6db369...0b09246. Read the comment docs.

@rsperer rsperer changed the title 1647 pedestrian city vs street accident 1647 pedestrian city vs street accident widget Feb 28, 2021
@rsperer rsperer marked this pull request as ready for review February 28, 2021 07:39
Copy link
Collaborator

@BarVolunteering BarVolunteering left a comment

Choose a reason for hiding this comment

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

The logic and the code are good, the only the thing that i am not sure about is the is_urban - I don't know if we have any other widgets that are urban, and how they are working(if they exist). - maybe someone else will know.

@zharpaz-bmc-com
Copy link

The logic and the code are good, the only the thing that i am not sure about is the is_urban - I don't know if we have any other widgets that are urban, and how they are working(if they exist). - maybe someone else will know.

@assafdayan
Regarding is_urban. Is there need for a specific predicate regarding urban location? Because the relevance to the news-flash is covered by the rules, i.e. ranking of urban widgets in locations that are not urban will be very low, so they will not be chosen.

@rsperer
Copy link
Author

rsperer commented Mar 6, 2021

The logic and the code are good, the only the thing that i am not sure about is the is_urban - I don't know if we have any other widgets that are urban, and how they are working(if they exist). - maybe someone else will know.

@assafdayan
Regarding is_urban. Is there need for a specific predicate regarding urban location? Because the relevance to the news-flash is covered by the rules, i.e. ranking of urban widgets in locations that are not urban will be very low, so they will not be chosen.

As at the time of implementation there was no rule based engine (is there now?) I wanted to refrain from checking location type in each widget as inter-urban and urban have mutually exclusive fields in the location. Once we have rules in place it can be removed.

Comment on lines +1691 to +1695
is_urban = is_location_inner_city(request_params.location_info)
for w in get_widget_factories():
widget: Widget = w(request_params)
if is_urban != widget.is_urban():
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Urban is the first categorization of a newsflash by which a widget decides whether it is relevant. Soon we will have many additional categories. The different categories should not appear on the infrastructure level. I have two suggestions until the ranking will be operational:
1 - Simply return empty items, and generate_widgets() will not enter it to the results
2 - instead of is_urban(location_info), use is_included(request_params)

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.

Widget: Total accidents involving pedestrians comparison - city vs specific street per accident severity
4 participants