-
Notifications
You must be signed in to change notification settings - Fork 1
Feat: 5-min data visualization UI #201
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
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
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.
Generally this is looking really good
- The introduction of
pems_streamlit/components
is a nice organization! - Renaming
stations/
todistricts/
makes a lot of sense!
I left a comment about caching -- I had misunderstood the data previously, and added a ttl
that doesn't make a lot of sense. Maybe we can increase that as part of this PR?
I think this is a good start and gets us some visualizations and new page elements to play with. I do believe there is more work to do to get these District landing pages working the way we ultimately want, so I filed #205 and added some comments to #202 as well.
In a follow-up / as we evolve more components, I might suggest a further organization into e.g.
components/maps/...
- e.g.
components/maps/station_summary.py
- e.g.
components/plots/...
- e.g.
components/plots/5m_traffic_data.py
- e.g.
- etc.
pems_streamlit/src/pems_streamlit/apps/districts/app_stations.py
Outdated
Show resolved
Hide resolved
3e7dab2
to
810f4dd
Compare
Thanks for the review @thekaveman! I agree that there's more work to do, I'll work on the followups next 👍 I guess we can probably get the AWS Redis PR merged first, then I can rebase this PR and after that get it merged if all the comments in this PR have been addressed. |
This is ready for a re-review whenever you get a chance @thekaveman, thanks! |
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.
This looks great!
One comment, you can decide if you want to address or not.
pems_streamlit/src/pems_streamlit/components/plot_5_min_traffic_data.py
Outdated
Show resolved
Hide resolved
ensure that the required parameters are set before proceeding with the visualization
the data table is not needed in the UI at this point
remove "greyed out" plot and error messages from the previous run. to ensure only one plot or error message is shown, draw it inside a dedicated placeholder.
place app_stations.py under districts to match pems_web and to use the more accurate streamlit app name "districts--stations"
setting the cache to 1 hour seems like a good balance between performance and data freshness.
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.
🚀 📈 📊
Closes #183
This PR builds out the UI (mostly Streamlit components) for the 5-min data visualization for traffic stations. It adds "map and station summary" and "5-min traffic data visualization" modules that can be imported by a Streamlit app. A small map of the station's location along with its metadata is shown on the UI. Data can be selected by quantity (speed, volume, occupancy), day, and lane. One or two quantities can be visualized in a line graph.
Follow-ups
districts/
behavior #203