51 create UI + version stabilization#61
Conversation
…ritization criteria improved (using frequency median instead of hard coded value)
…<1 lane/direction
…p_sequence==1 (consider first, ordered, instead)
There was a problem hiding this comment.
Pull request overview
This PR updates the package website/docs and real-time (GTFS-RT) workflow to support a dashboard/UI and stabilize/clarify the RT collection APIs.
Changes:
- Renames/splits GTFS-RT collection into
rt_collect_json()andrt_collect_protobuf()and updates docs/vignettes accordingly. - Extends lane/way frequency outputs with a
routescolumn (semicolon-separated route IDs) and uses it in downstream tooling. - Updates pkgdown site configuration (theme + navbar “Dashboard”), adds favicon/PWA assets, and introduces a dev script to generate dashboard-ready outputs.
Reviewed changes
Copilot reviewed 23 out of 36 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/rt.Rmd | Updates RT collection/visualization examples and adds a screenshot. |
| vignettes/prioritize.Rmd | Updates prioritization narrative + map logic and median-threshold example. |
| vignettes/articles/GTFShift.Rmd | Adds a “Key functions” landing article for pkgdown. |
| pkgdown/favicon/web-app-manifest-192x192.png | Adds PWA icon asset. |
| pkgdown/favicon/web-app-manifest-512x512.png | Adds PWA icon asset. |
| pkgdown/favicon/site.webmanifest | Adds web manifest referencing PWA icons. |
| pkgdown/favicon/favicon.ico | Adds favicon asset. |
| pkgdown/favicon/favicon-96x96.png | Adds favicon asset. |
| pkgdown/favicon/apple-touch-icon.png | Adds Apple touch icon asset. |
| man/rt_extend_prioritization.Rd | Updates references/examples for the new RT collection functions. |
| man/rt_collect_protobuf.Rd | Updates title/details wording for protobuf collection. |
| man/rt_collect_json.Rd | Renames documentation to rt_collect_json() and updates examples. |
| man/prioritize_lanes.Rd | Documents newly returned routes column. |
| man/get_way_frequency_hourly.Rd | Documents newly returned routes column. |
| dev/web_version.R | New script to generate dashboard-ready outputs + metadata. |
| dev/test_rt.R | Updates dev RT collection usage and file naming. |
| _pkgdown.yml | Adds theme tweaks + navbar “Dashboard” link. |
| README.md | Refreshes README content, adds dashboard promotion and links. |
| R/rt_extend_prioritization.R | Updates RT docs and validation logic (but currently introduces a critical geometry issue). |
| R/rt_collect_protobuf.R | Adjusts protobuf collector to call rt_collect_json(). |
| R/rt_collect_json.R | Renames rt_collect() to rt_collect_json() and updates docs. |
| R/query_osm_shapes_to_routes.R | Improves logging around matched/missing shapes/routes. |
| R/query_osm_shapes_match_routes.R | Fixes first-stop selection by ordering stop_sequence. |
| R/query_osm_bus_lanes.R | Guards if_any() checks when tag columns are absent. |
| R/prioritize_lanes.R | Adds routes to returned columns and adjusts lane direction heuristics. |
| R/get_way_frequency_hourly.R | Computes first-stop departures more robustly; adds routes aggregation. |
| R/get_route_frequency_hourly.R | Computes first-stop departures more robustly. |
| NAMESPACE | Exports rt_collect_json instead of rt_collect. |
Comments suppressed due to low confidence (4)
vignettes/rt.Rmd:47
- This vignette references
GTFS::rt_collect_protobuf(), but the function appears to live in this package (GTFShift::rt_collect_protobuf()). Please fix the package qualifier to avoid pointing readers to a non-existent/incorrect function.
R/rt_extend_prioritization.R:52 rt_extend_prioritization()later bufferslanes_uniquewithsf::st_transform()/sf::st_buffer(), which requireslanes_uniqueto retain the sf geometry. Withrequired_colsreduced to onlyway_osm_id(and withlanes_uniquebeing reduced to justway_osm_id),lanes_uniquewill not be an sf object and the buffering step will error. Keepgeometryas a required column and preserve it when buildinglanes_unique(e.g., distinct onway_osm_idwhile keeping geometry).
# 1. Validate inputs
required_cols = c("way_osm_id")
missing_cols = setdiff(required_cols, colnames(lane_prioritization))
if (length(missing_cols) > 0) {
stop(paste("lane_prioritization is missing required columns:", paste(missing_cols, collapse = ", ")))
}
R/rt_extend_prioritization.R:31
- The examples imply
rt_collect_json()produceslongitude/latitudecolumns, but with the defaultfields_collectit writes columns named likevehicle.position.longitudeandvehicle.position.latitude. As written,st_as_sf(coords = c("longitude", "latitude"))will fail unless the caller customizesfields_collect(or renames columns). Please update the example to match the default output (or showfields_collect/rename explicitly).
#' \dontrun{
#' rt_collect_file <- "gtfs_rt_data.csv"
#' GTFShift::rt_collect_json("https://api.example.com/gtfs-rt", rt_collect_file)
#' lane_prioritization <- GTFShift::prioritize_lanes(gtfs, osm_query)
#'
#' rt_collection <- read.csv(rt_collect_file) |> sf::st_as_sf(coords = c("longitude", "latitude"), crs = 4326)
#' lane_prioritization_extended <- GTFShift::rt_extend_prioritization(
R/rt_collect_json.R:33
- Renaming the exported function from
rt_collect()tort_collect_json()is a breaking change for existing users. Consider keeping a backwards-compatiblert_collect()wrapper that callsrt_collect_json()and emits a deprecation warning, and keep exporting it for at least one release cycle.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gtfs = character(), | ||
| query = I(list()), | ||
| rt_interval = character(), | ||
| rt_collection = I(list()) # sf object | ||
|
|
There was a problem hiding this comment.
regions is initialized with columns gtfs (but not gtfs_url/gtfs_day), then rbind() adds rows with different column names (gtfs_url, gtfs_day). This creates inconsistent/misaligned columns (and makes later region$gtfs_url/region$gtfs_day fragile). Align the initial regions schema with the fields you actually use (gtfs_url, gtfs_day, etc.).
| gtfs = character(), | |
| query = I(list()), | |
| rt_interval = character(), | |
| rt_collection = I(list()) # sf object | |
| gtfs_url = character(), | |
| gtfs_day = character(), | |
| query = I(list()), | |
| rt_interval = character(), | |
| rt_collection = I(list()) # sf object |
| To collect GTFS-RT data, use `GTFShift::rt_collect_json()` or `GTFShift::rt_collect_protobuf()`, depending on the encoding the feed uses (JSON or Protocol Buffers, respectively). These fetch GTFS-RT data from the endpoint and save it to a CSV file for further analysis. The method runs in a loop, fetching data at regular intervals (default is every 60 seconds) until manually stopped (CTRL+C). | ||
|
|
||
| ```{r, eval=FALSE} | ||
| rt_collect_file <- "gtfs_rt_data.csv" | ||
| GTFShift::rt_collect("https://api.example.com/gtfs-rt", rt_collect_file) | ||
| GTFShift::rt_collect_json("https://api.example.com/gtfs-rt", rt_collect_file) | ||
| ``` |
There was a problem hiding this comment.
rt_collect_json() is described here as saving GTFS-RT data to CSV for later st_as_sf() use. With the current default fields_collect, the written column names are dot-paths (e.g., vehicle.position.longitude/latitude), so downstream examples that use coords = c("longitude","latitude") won’t work unless fields_collect is adjusted or columns are renamed. Consider clarifying this here (or updating defaults/examples for consistency).
dev/web_version.R
Outdated
| region <- regions[i, ] | ||
| message(sprintf("\n\nRunning for %s (%s)...", region$name, region$gtfs_day)) | ||
|
|
||
| output_region = sprintf("%s/%s/%s", output, regions$name, region$gtfs_day) |
There was a problem hiding this comment.
output_region = sprintf("%s/%s/%s", output, regions$name, region$gtfs_day) uses regions$name (the full vector) instead of region$name (the current row). This will produce a vector of paths and break dir.create()/file writes inside the loop. Use the current region$name here.
| output_region = sprintf("%s/%s/%s", output, regions$name, region$gtfs_day) | |
| output_region = sprintf("%s/%s/%s", output, region$name, region$gtfs_day) |
vignettes/prioritize.Rmd
Outdated
| - `GTFShift::rt_collect_json()` or `GTFShift::rt_collect_protobuf()`, to collect GTFS-RT data, which can be later used with `GTFShift::rt_extend_prioritization()` to include real-time operational metrics in the prioritization analysis. | ||
|
|
||
| This document explores how to use these methods in a combined way to assist public transport planners in prioritizing bus lane implementations. For details on the several encapsulated features, refer to the numerated articles in the menu, that explore in detail each of the specific approaches followed. | ||
| This document explores how to use these methods in a combined way to assist public transport planners in prioritizing bus lane implementations. For details on the several encapsulated features and method variations, refer to the numerated articles in the menu, that explore in detail each of the specific approaches followed. |
There was a problem hiding this comment.
Typo: “numerated articles” should be “numbered articles”.
README.md
Outdated
| ## Get started | ||
|
|
||
| ### OSM Data | ||
| For more details on the package and how to get started, please visit the [Get started](./articles/GTFShift.html) page. |
There was a problem hiding this comment.
The README link [Get started](./articles/GTFShift.html) is broken on GitHub (there is no articles/ directory in the repository). Consider linking to the pkgdown site URL instead (e.g., https://u-shift.github.io/GTFShift/articles/GTFShift.html).
| For more details on the package and how to get started, please visit the [Get started](./articles/GTFShift.html) page. | |
| For more details on the package and how to get started, please visit the [Get started](https://u-shift.github.io/GTFShift/articles/GTFShift.html) page. |
| ) + mapview::mapview( | ||
| lanes |> filter(!is_bus_lane & frequency>=5 & !is.na(n_lanes) & n_lanes_direction>1), | ||
| layer.name="NO bus lane with 5 or + bus/h + 1 lane/dir", | ||
| color="#F63049" | ||
| lanes_0800 |> filter(!is_bus_lane & frequency>=p50_frequency & !is.na(n_lanes) & n_lanes_direction>1), | ||
| layer.name=sprintf("NO bus lane with +%d bus/h +1 lane/dir", p50_frequency-1), | ||
| color="#F63049", |
There was a problem hiding this comment.
p50_frequency is computed via quantile() and may be non-integer; using %d in sprintf() truncates and can make the legend text misleading. Also the label uses p50_frequency-1 even though the filter threshold is frequency>=p50_frequency. Consider formatting/rounding consistently and reflecting the actual threshold in the label.
README.md
Outdated
| **GTFShift** is developed and maintained by | ||
| [U-shift](https://ushift.tecnico.ulisboa.pt) urban mobility research | ||
| group, part of [CERIS](https://ceris.pt/) research unit, at [Instituto | ||
| Superior Técnico](https://tecnico.ulisboa.pt/pt/), Lisbon, Portugal. | ||
| [U-Shift](https://ushift.tecnico.ulisboa.pt){target="_blank"} urban mobility research | ||
| group, part of [CERIS](https://ceris.pt/){target="_blank"} research unit, at [Instituto | ||
| Superior Técnico](https://tecnico.ulisboa.pt/pt/){target="_blank"}, Lisbon, Portugal. |
There was a problem hiding this comment.
{target="_blank"} is not supported by GitHub Markdown and will render literally in the README for these links as well. Consider removing it (or switching to HTML anchors).
| @@ -19,7 +19,7 @@ export(osm_shapes_to_routes) | |||
| export(osm_trips_to_routes) | |||
| export(prioritize_lanes) | |||
| export(query_mobilitydatabase) | |||
There was a problem hiding this comment.
If you add a deprecated rt_collect() wrapper for backwards compatibility, it also needs to remain exported here (currently only rt_collect_json is exported). Otherwise, existing code calling GTFShift::rt_collect() will fail after updating.
| export(query_mobilitydatabase) | |
| export(query_mobilitydatabase) | |
| export(rt_collect) |
README.md
Outdated
| Visit it at [ushift.pt/apps/gtfshift](https://ushift.pt/apps/gtfshift){target="_blank}. | ||
|
|
||
| > OSM exported bus lanes for Lisbon | ||
| [](https://ushift.pt/apps/gtfshift){target="_blank"} | ||
|
|
There was a problem hiding this comment.
GitHub-flavored Markdown doesn’t support the {target="_blank"} attribute syntax, so these will render as literal text in the README. If you need new-tab behavior in pkgdown only, omit it here; otherwise use an explicit HTML <a target="_blank" ...> link.
README.md
Outdated
| - [`{tidytransit}`](https://github.com/r-transit/tidytransit){target="_blank"} | ||
| - [`{gtfstools}`](https://github.com/ipeaGIT/gtfstools/){target="_blank"} | ||
| - [`{GTFSwizard}`](https://github.com/nelsonquesado/GTFSwizard){target="_blank"} | ||
| - [`{gtfsrouter`}](https://github.com/UrbanAnalyst/gtfsrouter){target="_blank"} |
There was a problem hiding this comment.
Same issue here: {target="_blank"} will not be interpreted on GitHub and will appear in the rendered README. Prefer plain Markdown links or HTML anchors if the target attribute is required.
No description provided.