-
Notifications
You must be signed in to change notification settings - Fork 8
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
RODARS pipeline #1103
RODARS pipeline #1103
Conversation
Ready to review! Will add some more usage examples to readme at some point. |
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.
Commenting on the documentation.
Pleased that the docs are extensive, you have thought out the limitations of the data, done some exploratory analysis and the use of the callouts for extra punchy information.
I have some thoughts on re-ordering the README, but maybe we merge this and solicit feedback from users.
I have added some small editorial comments on language.
I think the big thing missing in terms of documentation is that parent folders should have a brief description of the contents of this folder so that people can know where to find it.
Also should this folder be more accurately called something like lane_obstructions
? Or road_permits
? Since the data are for events and construction (right?)
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.
just some v smol clean up request, :gabe-approved:
events/construction/sql/create-table-itsc_factors_lanesaffectedpattern.sql
Outdated
Show resolved
Hide resolved
events/construction/sql/create-function-get_lanesaffected_sum.sql
Outdated
Show resolved
Hide resolved
CASE iil.centreline_id WHEN 0 THEN NULL ELSE iil.centreline_id END AS centreline_id, | ||
COALESCE( | ||
centreline_latest.geom, | ||
--find geoms for centreline that do not appear in centreline_latest |
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.
ohhhh nice
7f407b0
to
0e11ccf
Compare
0e11ccf
to
d58eab4
Compare
@radumas readme changes ready for re-review:
|
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.
Looks great! Thank you!
Is there a bug in the linting action that it isn't annotating? |
I also noticed previously on this PR there were some lines being erroneously annotated with line length sqlfluff error. Perhaps time to look into upgrading sqlfluff action. |
What this pull request accomplishes:
congestion_events.rodars_locations
is the main view we will be using!rodars_pull.py
daily DAG which runs on Morbius.rodars_functions.py
, including: unnesting json data, converting binary coordinates to postgres readable format, data type conversion.congestion_events.rodars_issues
andcongestion_events.rodars_issue_locations
.on conflict do update
.itsc_factors
schema:direction
,lanesaffectedpattern
,locationblocklevel
,roadclosuretype_new
,roadclosuretype_old
. Used to convert codes to human readable format inissue_locations
view.itsc_factors.lanesaffectedpattern
lookup could use review: I devised thelane_open
,lane_closed
columns to give a numeric interpretation of how many lanes are open/closed. I used 0.5 for partial closures/slowdowns.itsc_factors.get_lanesaffected_sums
which translates these codes into numeric columns for ease of use in the main view. Alanesaffectedpattern
could be something like'LOLCLCWO'
(lane open, lane closed, lane closed, sidwalk open) (Try:SELECT lane_open_auto, lane_closed_auto, lane_open_bike, lane_closed_bike, lane_open_ped, lane_closed_ped, lane_open_bus, lane_closed_bus FROM itsc_factors.get_lanesaffected_sums('LOLCLCWO')
)Issue(s) this solves:
What, in particular, needs to reviewed:
congestion_events
)? View/table names?What needs to be done by a sysadmin after this PR is merged