960 function to create gis file new point aus for submission to attains

[hillarymarler]

closes #960

Added a new function TADA_CreatePointAUGeometry and some helper functions. Made some minor changes to TADA_CrosswalkATTAINSWaterTypes and TADA_CreatePointAUs to facilitate the addition of TADA_CreatePointAUGeometry to the workflow. These new functions will be demonstrated n a future, updated mod 2 workflow vignette.

Next steps will be to create functions to generate the ATTAINS batch upload csvs for Assessment Units and Assessment Units with Monitoring Locations (this can be built off of TADA_CreateATTAINSAUMLCrosswalk).

It would be great to get #964 merged first. Then I can double check that the addition of org_id as a param for TADA_CrosswalkATTAINSWaterTypes does not have any impact on these changes. I do not expect it to.

Pull Request Checklist (convert PR to draft if in progress)

Required

• Update your branch from the latest develop and resolve any merge conflicts

• Run devtools::test(), devtools::check(), and devtools::document() locally; ensure tests pass and fix any errors, warnings, or notes. Add new dependencies to DESCRIPTION and document appropriately

• 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

• Request review from at least one developer team member (convert PR to ready for review if it was designated as in progress)

Best practices

• Include a summary of the changes made and relevant context/motivation

• Link issues to auto-close on merge (use Development sidebar or include "Closes #" in the PR)

• Refresh inline/block comments for clarity

• Update roxygen docs and include examples; review help pages

• Add/update tests in tests/testthat; review the bot's coverage report from test-coverage and confirm all changes are covered

Conditional

• If there is a bot spelling comment, run spelling::spell_check_package() locally and fix any misspellings; add approved project terms to WORDLIST with spelling::update_wordlist()

• If tests fail suggesting internal reference files need a refresh, run .TADA_UpdateRefFiles() and .TADA_UpdateExampleData() locally via MaintenanceScheduled.R or trigger the Component File Update GitHub Action

• If new example data files were added, document them in ExampleData.R and include them in MaintenanceScheduled.R for regular refresh

• If columns were added/updated, update RequiredCols.R

• If changes affect other package or the shiny app functions, update those impacted functions accordingly

[hillarymarler]

I am setting this as draft until #964 is merged as there is a test failure here I expect will be resolved by change sin #964.

[hillarymarler]

Thanks for all of the detailed comments and suggestions @cristinamullin and @jbousquin. I am converting this to draft while I address them.

[hillarymarler]

(1) TADA_CrosswalkATTAINSWaterTypes(): The verb "crosswalk" suggests mapping equivalent fields across two tables, generally I'd expect the result of this being the crosswalk itself. For TADA_CrosswalkATTAINSWaterTypes() it sounds like you are using the crosswalk (internal ref) to add/update ATTAINS.WaterType from TADA.MonitoringLocationTypeName. Would the verb "update" work instead? i.e., TADA_UpdateATTAINSWaterTypes(). That would give space for both "add" where missing and "replace".
Update: that naming makes aligns it w/ TADA_UpdateATTAINSAUMLCrosswalk() as well.

I like TADA_UpdateATTAINSWaterTypes() for consistency

I changed the function name from "TADA_CrosswalkATTAINSWaterTypes()" to to "TADA_UpdateATTAINSAUMLCrosswalk()"

[hillarymarler]

(3) TADA_CreatePointAUs() & (4) TADA_CreatePointAUGeometry() - My inclination is to get more specific as you move to the right, e.g., TADA_CreateAUPoints() & TADA_CreateAUPointGeometry(). Tradeoff is the abbreviation and camel-case doesn't look as clean.

TADA_CreatePointAUs is in ATTAINSCrosswalks and TADA_CreatePointAUGeometry is in GeospatialFunctions because TADA_CreatePointAUs is creating an ATTAINS AU–ML Crosswalk from WQP Monitoring Location IDs. But there are other crosswalk generating functions in geospatialFunctions so it might merit coming up with standards for what defaults to where (as a new issue). What are your thoughts on that and doing it with just a Script/Module level header comment? (i.e., meant for devs not users so it doesn't need to be grabbed by roxygen for docs), e.g.,:

# -----------------------------------------------------------------------------
# Title: Geospatial Functions
# Description: Functions whose focus is on fetching data spatially or
#                     other direct spatial manipulations.
# -----------------------------------------------------------------------------

@cristinamullin what do you think of these suggestions for function renaming? I like Justin's suggestion to get more specific as we move to the right in function names. If you agree, I will change them.

[mhweber]

Is there a reason that 1) the default is c:/Users/username/Downloads is hard-wired as the download location using .get_downloads_path and that a user-chosen location couldn't be a parameter? 2) a zipped shapefile is the default format? Perhaps hardwiring these two things to downloads folder and zipped shapefile makes sense for the scope of this function, but as a naive user my immediate question is could I choose a geopackage as download format which I prefer and could I choose where to download?

[hillarymarler]

I followed @wokenny13's lead from his mod 3 work on downloaded file location. But I do like the idea of incorporating the option for a user-chosen location anytime EPATADA facilitates a download. The shp file format was @jesseboormanpadgett's suggestion for ease of use with ATTAINS, but I like your suggestion to facilitate downloading as a geopackage too as some users are likely to prefer that.

I'll add these topics to the agenda for the next team meeting.

[mhweber]

A general comment that is broader than the scope of this PR but just making a note of here is I think we might consider some housekeeping of which functions go in which .R files and possibly consolidating / renaming some of the .R files? For instance, we have GeospatialFunctions.R and GeospatialUtilities.R - is there a reason they should be separate? Or do functions in GeospatialUtilities.R need to be kept separate from functions in Utilities.R? And some of the functions in GeospatialFunctions.R seem like they might make more sense pulled out in an Attains R file?

[hillarymarler]

A general comment that is broader than the scope of this PR but just making a note of here is I think we might consider some housekeeping of which functions go in which .R files and possibly consolidating / renaming some of the .R files? For instance, we have GeospatialFunctions.R and GeospatialUtilities.R - is there a reason they should be separate? Or do functions in GeospatialUtilities.R need to be kept separate from functions in Utilities.R? And some of the functions in GeospatialFunctions.R seem like they might make more sense pulled out in an Attains R file?

I would like to discuss this as well as function naming conventions at a internal team meeting soon.

[hillarymarler]

Updates I still need to make on this PR:

• allowable flag - This allowable flag is assigned 3x, consider setting it as a var (e.g., allowable_value_flag) up top. That way you don't have to type it out, ensures it is consistent, and makes it easier to maintain. Bonus points if this follows a standard package flag convention and there is an internal glue/c style function to create the flag for you (I've not checked if such a function already exists).

I set the allowable value flag as a var, but have not yet created an internal function.

• TADA.ATTAINSWaterType.Flag - Make these two flags more similar, e.g., ""ATTAINS.WaterType was updated to NA"

I made the flag language more similar.

• flag/update workflow (see line 4735-4738() - It seems like if flag or update it's going to assign the same flag to matched rows. Could that just be pulled out and done up front (un-nested)? That way the conditionals would only run on the filtered subset where there isn't a match.

I refactored the function so there isn't duplicated effort.

• update function names so specificity increases you move to the right - e.g., TADA_CreateAUPoints() & TADA_CreateAUPointGeometry()