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

2604 location accuracy #2641

Merged
merged 3 commits into from
May 7, 2024
Merged

Conversation

ziv17
Copy link
Collaborator

@ziv17 ziv17 commented May 1, 2024

Add location accuracy filter according to the current resolution.
The addition is made in get_accidents_stats where possible.

@ziv17 ziv17 force-pushed the 2604-location-accuracy branch 3 times, most recently from 418fdad to 76850b8 Compare May 2, 2024 03:50
@atalyaalon atalyaalon marked this pull request as ready for review May 5, 2024 14:50
@atalyaalon
Copy link
Collaborator

@ziv17 can you prod merge dev into this branch? I want to examine in the pr only the relevant changes for this issue

@atalyaalon atalyaalon linked an issue May 5, 2024 that may be closed by this pull request
ziv17 added 2 commits May 5, 2024 21:23
Add resolution parameter to get_accidents_stats() to add the location accuracy filter.
In widgets that do not use it, handle the filter dicts that is passed to get_quary, or the filter directly.
@ziv17 ziv17 force-pushed the 2604-location-accuracy branch from 76850b8 to fef1db0 Compare May 5, 2024 18:29
@ziv17
Copy link
Collaborator Author

ziv17 commented May 5, 2024

@ziv17 can you prod merge dev into this branch? I want to examine in the pr only the relevant changes for this issue

I rebased on top of upstream/dev.

Copy link
Collaborator

@atalyaalon atalyaalon left a comment

Choose a reason for hiding this comment

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

@ziv17 great work! All in all the PR looks good!
I wonder if there is a need to add location_accuracy as index for the CBS View tables.
I think the answer is no for now, since it's always queried with other location variables that are indexed.
However, if we see that this change impacts the running time of the cache update flows, we'll reconsider.

Hence the only thing left to change is the BackEnd Consts comment, and then I'll merge.

Great work! Thanks!

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 88.77551% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 53.52%. Comparing base (400af13) to head (b26f4b2).
Report is 21 commits behind head on dev.

Files Patch % Lines
...cident_type_vehicle_type_road_comparison_widget.py 42.85% 4 Missing ⚠️
anyway/widgets/widget_utils.py 91.83% 4 Missing ⚠️
...dgets/injured_accidents_with_pedestrians_widget.py 0.00% 2 Missing ⚠️
...ed_and_injured_count_per_age_group_widget_utils.py 90.90% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2641      +/-   ##
==========================================
+ Coverage   53.22%   53.52%   +0.30%     
==========================================
  Files         119      119              
  Lines        9924     9956      +32     
==========================================
+ Hits         5282     5329      +47     
+ Misses       4642     4627      -15     
Flag Coverage Δ
unittests 53.52% <88.77%> (+0.30%) ⬆️

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.

@atalyaalon atalyaalon merged commit e4beaa2 into data-for-change:dev May 7, 2024
3 checks passed
@ziv17 ziv17 deleted the 2604-location-accuracy branch May 9, 2024 18:24
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.

Filter Location Accuracy in Widgets queries
3 participants