-
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
1085 Ecocounter Open Data Schema+DAG+QC plots #1096
base: master
Are you sure you want to change the base?
1085 Ecocounter Open Data Schema+DAG+QC plots #1096
Conversation
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.
Only commenting on the OpenData schema.
Looking pretty solid.
To summarize some change requests made elsewhere:
- There should be README updates in here for documentation relevant to the unit.
- the VIEWs the DAG are pulling from could be in
open_data
and named to include Miovision data. When Miovision is ready to publish, we can replace the VIEWs to have thoseUNION
s without having to change the DAG - the unit of location should be the linear direction (directional centreline) to
- handle technologies having different centrelines for each direction, or
- handle one side of the street being faulty; and,
- avoid the public having to join multiple location tables.
- And we don't expose the "parent" site/sensor to the public.
@@ -106,46 +96,93 @@ def reminder_message(ds = None, **context): | |||
wait_till_10th.doc_md = """ | |||
Wait until the 10th day of the month to export data. Alternatively mark task as success to proceed immediately. | |||
""" | |||
|
|||
@task() | |||
def get_years(ds=None): |
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.
did we add this for mapped task naming? It was using ds before 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.
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.
OK to remove sensitivity_changes
since it's superseded by sensitivity_history
.
Not sure why flow_id = 101042942
has a stray date_range = (,)
.
The readme should mention:
sensitivity_history
is manually updated based on emails with with Eco-counter Technical Support Specialist (Derek Yates as of 2024).- what the numbered settings mean (least to most selective, or not standardized)
- Eco-counter must confirm what the current and new settings are whenever a sensitivity is adjusted, along with the date the change is applied.
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.
OK to remove sensitivity_changes since it's superseded by sensitivity_history.
✅
Not sure why flow_id = 101042942 has a stray date_range = (,) .
That flow_id never had any data. Deleted the entry.
The readme should mention...
Adding!
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.
How to separate scooter flows calibration factors from bike flows? Currently we apply the same "total volume" factor to both, but we will have separate validation factors for these veh types soon. (Not a blocker for this release)
Readme should state:
ecocounter.calibration_factors
excludes factors inecocounter.manual_counts_info
with flagged asdo_not_use
=true
.do_not_use
is entered manually on a case-by-case basis depending on which calibration study is the 'best'.- Caution is required when assigning
do_not_use
to calibration studies. Avoid as much as possible applying a new factor to data that has already been calibrated and published to Open Data.
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.
The way to separate scooter calibration factors would be to edit ecocounter.validation_results
, particularly the flow_ids
CTE to link different flow_ids to those studies.
@gabrielwol, I'm not finding this file that my review was requested for: volumes/ecocounter/views/create-view-counts_calibrated.sql |
Renamed to "counts" from counts_calibrated |
In bigdata, I see both VIEWs |
@A-DUYVESTYN yes, we would keep only
Will work on readmes today. |
OK. I'm not clear on process for assigning |
5bbc065
to
c4aa662
Compare
… airflow imports (#1115)
c4aa662
to
ee775d3
Compare
What this pull request accomplishes:
New Monthly DAG to insert/download Ecocounter data. Includes prompts to review data and check if data for all days are present. @chmnata
Open Data tables + functions @A-DUYVESTYN
open_data_daily_counts
+ fnopen_data_daily_counts_insert
,open_data_raw_counts
+ fnopen_data_raw_counts_insert
,open_data_sites
Calibration factor supporting tables @A-DUYVESTYN
sensitivity_changes
when we are satisfied with changes.Does not need significant review:
R plots
qc_graph_volumes
postgres functionMisc.
counter
to sites table (commonly used ID field in Ecocounter communications/dashboard)ecocounter_pull.py
,pull_data_from_api.py
.direction_main
field to flows: easily group dominant/contraflow togethercounts
,counts_calibrated
Issue(s) this solves:
What, in particular, needs to reviewed:
What needs to be done by a sysadmin after this PR is merged
counts_old
(saved the old version of counts for monitoring project dependency)discontinuities
table