Update TADA_CreateAUMLCrosswalk #769
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Key additions: Guarding against NULLs and vector org_id values, and explicitly defaulting org_id = "all" when org_id is NULL (common in Shiny) so the function will query ATTAINS unless org_id is explicitly set to "none". Preflight connectivity check to ATTAINS with timing and more informative messages. Increased timeout and timing instrumentation around the ATTAINS web-service calls to make their latency visible. tryCatch around remote/geospatial calls to avoid hard failures on server and to surface diagnostic messages. Guards around sf operations and missing coordinates.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
air
[air] reported by reviewdog 🐶
EPATADA/R/GeospatialFunctions.R
Line 3925 in 4d704a8
[air] reported by reviewdog 🐶
EPATADA/R/GeospatialFunctions.R
Line 3927 in 4d704a8
[air] reported by reviewdog 🐶
EPATADA/R/GeospatialFunctions.R
Line 3936 in 4d704a8
[air] reported by reviewdog 🐶
EPATADA/R/GeospatialFunctions.R
Lines 4073 to 4075 in 4d704a8
[air] reported by reviewdog 🐶
EPATADA/R/GeospatialFunctions.R
Line 4085 in 4d704a8
[air] reported by reviewdog 🐶
Lines 528 to 530 in 4d704a8
[air] reported by reviewdog 🐶
Lines 92 to 93 in 4d704a8
[air] reported by reviewdog 🐶
Lines 323 to 325 in 4d704a8
[air] reported by reviewdog 🐶
Line 2363 in 4d704a8
[air] reported by reviewdog 🐶
Lines 277 to 279 in 4d704a8
[air] reported by reviewdog 🐶
EPATADA/tests/testthat/test-URLChecker.R
Lines 103 to 105 in 4d704a8
R/CriteriaComparison.R
Outdated
Comment on lines
430
to
432
| all.pairs <- pair.activityid |> | ||
| rbind(pair.ml.time) |> | ||
| dplyr::distinct() |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| all.pairs <- pair.activityid |> | |
| rbind(pair.ml.time) |> | |
| dplyr::distinct() | |
| all.pairs <- pair.activityid |> rbind(pair.ml.time) |> dplyr::distinct() |
R/DataDiscoveryRetrieval.R
Outdated
Comment on lines
380
to
381
| ~tribal_area, ~url, | ||
| "Alaska Native Allotments", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/0", |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| ~tribal_area, ~url, | |
| "Alaska Native Allotments", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/0", | |
| ~tribal_area , ~url , | |
| "Alaska Native Allotments" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/0" , |
R/DataDiscoveryRetrieval.R
Outdated
Comment on lines
383
to
385
| "American Indian Reservations", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/2", | ||
| "Off-reservation Trust Lands", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/3", | ||
| "Oklahoma Tribal Statistical Areas", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/4" # , |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| "American Indian Reservations", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/2", | |
| "Off-reservation Trust Lands", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/3", | |
| "Oklahoma Tribal Statistical Areas", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/4" # , | |
| "American Indian Reservations" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/2" , | |
| "Off-reservation Trust Lands" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/3" , | |
| "Oklahoma Tribal Statistical Areas" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/4" # , |
R/DataDiscoveryRetrieval.R
Outdated
Comment on lines
904
to
906
| record_count <- query_avail |> | ||
| dplyr::pull(resultCount) |> | ||
| sum() |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| record_count <- query_avail |> | |
| dplyr::pull(resultCount) |> | |
| sum() | |
| record_count <- query_avail |> dplyr::pull(resultCount) |> sum() |
R/DataDiscoveryRetrieval.R
Outdated
Comment on lines
1104
to
1110
| ~tribal_area, ~url, | ||
| "Alaska Native Allotments", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/0", | ||
| "Alaska Native Villages", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/1", | ||
| "American Indian Reservations", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/2", | ||
| "Off-reservation Trust Lands", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/3", | ||
| "Oklahoma Tribal Statistical Areas", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/4", | ||
| "Virginia Federally Recognized Tribes", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/5" |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| ~tribal_area, ~url, | |
| "Alaska Native Allotments", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/0", | |
| "Alaska Native Villages", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/1", | |
| "American Indian Reservations", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/2", | |
| "Off-reservation Trust Lands", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/3", | |
| "Oklahoma Tribal Statistical Areas", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/4", | |
| "Virginia Federally Recognized Tribes", "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/5" | |
| ~tribal_area , ~url , | |
| "Alaska Native Allotments" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/0" , | |
| "Alaska Native Villages" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/1" , | |
| "American Indian Reservations" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/2" , | |
| "Off-reservation Trust Lands" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/3" , | |
| "Oklahoma Tribal Statistical Areas" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/4" , | |
| "Virginia Federally Recognized Tribes" , "https://geopub.epa.gov/arcgis/rest/services/EMEF/Tribal/MapServer/5" |
R/GeospatialFunctions.R
Outdated
| crs = 4326 | ||
| # ADDED: Guard against missing coordinates which can occur if inputs have NA lat/long | ||
| # This avoids errors in st_as_sf on Shiny servers with incomplete data | ||
| if ("TADA.LongitudeMeasure" %in% names(user) && "TADA.LatitudeMeasure" %in% names(user)) { |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| if ("TADA.LongitudeMeasure" %in% names(user) && "TADA.LatitudeMeasure" %in% names(user)) { | |
| if ( | |
| "TADA.LongitudeMeasure" %in% | |
| names(user) && | |
| "TADA.LatitudeMeasure" %in% names(user) | |
| ) { |
R/GeospatialFunctions.R
Outdated
| # This avoids errors in st_as_sf on Shiny servers with incomplete data | ||
| if ("TADA.LongitudeMeasure" %in% names(user) && "TADA.LatitudeMeasure" %in% names(user)) { | ||
| user <- user |> | ||
| dplyr::filter(!is.na(.data$TADA.LongitudeMeasure), !is.na(.data$TADA.LatitudeMeasure)) |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| dplyr::filter(!is.na(.data$TADA.LongitudeMeasure), !is.na(.data$TADA.LatitudeMeasure)) | |
| dplyr::filter( | |
| !is.na(.data$TADA.LongitudeMeasure), | |
| !is.na(.data$TADA.LatitudeMeasure) | |
| ) |
R/GeospatialFunctions.R
Outdated
| attains, | ||
| coords = c("TADA.LongitudeMeasure", "TADA.LatitudeMeasure"), | ||
| crs = 4326 | ||
| if ("TADA.LongitudeMeasure" %in% names(attains) && "TADA.LatitudeMeasure" %in% names(attains)) { |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| if ("TADA.LongitudeMeasure" %in% names(attains) && "TADA.LatitudeMeasure" %in% names(attains)) { | |
| if ( | |
| "TADA.LongitudeMeasure" %in% | |
| names(attains) && | |
| "TADA.LatitudeMeasure" %in% names(attains) | |
| ) { |
R/GeospatialFunctions.R
Outdated
| crs = 4326 | ||
| if ("TADA.LongitudeMeasure" %in% names(attains) && "TADA.LatitudeMeasure" %in% names(attains)) { | ||
| attains <- attains |> | ||
| dplyr::filter(!is.na(.data$TADA.LongitudeMeasure), !is.na(.data$TADA.LatitudeMeasure)) |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| dplyr::filter(!is.na(.data$TADA.LongitudeMeasure), !is.na(.data$TADA.LatitudeMeasure)) | |
| dplyr::filter( | |
| !is.na(.data$TADA.LongitudeMeasure), | |
| !is.na(.data$TADA.LatitudeMeasure) | |
| ) |
R/GeospatialFunctions.R
Outdated
Comment on lines
3918
to
3921
| get.attains <- tryCatch(sf::st_make_valid(get.attains), error = function(e) { | ||
| message("st_make_valid (get.attains) failed: ", conditionMessage(e)) | ||
| get.attains | ||
| }) |
Contributor
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| get.attains <- tryCatch(sf::st_make_valid(get.attains), error = function(e) { | |
| message("st_make_valid (get.attains) failed: ", conditionMessage(e)) | |
| get.attains | |
| }) | |
| get.attains <- tryCatch( | |
| sf::st_make_valid(get.attains), | |
| error = function(e) { | |
| message("st_make_valid (get.attains) failed: ", conditionMessage(e)) | |
| get.attains | |
| } | |
| ) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Preparation
Update your branch from the latest
developand resolve any merge conflictsBefore creating a pull request trigger the format-update GitHub Action on your branch to format the code
Documentation
Add/update inline and/or block comments to clarify complexity, context and intent
Add/update function documentation (roxygen), include working examples, build docs locally using devtools::document(), and inspect added/updated help pages for content and format
Add/update vignettes for corresponding changes in functionality, list these under articles in _pkgdown.yml, and ensure added/updated vignettes run and build with proper formatting locally
Maintenance & Data Refresh
Add new dependencies to
DESCRIPTIONand document appropriatelyRun spelling maintenance in
requiredMaintenance.RIf changes affect other package or the shiny app functions, update those impacted functions accordingly
If columns were added/updated, add/update them in
RequiredCols.RRun
.TADA_UpdateRefFiles()and.TADA_UpdateExampleData()locally viaMaintenanceScheduled.Ror trigger the Component File Update GitHub ActionIf new example data files were added, document them in
ExampleData.Rand include them inMaintenanceScheduled.Rfor regular refreshTests and checks
Add/update tests in
tests/testthatto cover changesRun
devtools::test()to verify new and existing tests passRun
devtools::check()and address any errors, warnings or notesPull Request Description
Includes a summary of the changes made
Includes relevant context/motivation
Includes links to related issues or pull requests (keywords like "Closes #issue_number" automatically close related issues when pull request is merged)
Review
Review the bot-commented coverage-report generated by test-coverage to confirm all changes are covered by tests
Review/accept any bot-suggested format changes
Request review from at least one developer team member