482 create tadamonitoringlocationidentifier in tada autoclean #560
Conversation
Using TADA.MonitoringLocationIdentifier in TADA_FindNearbySites, changes made up through line 837
Update TADA_FindNearby to use TADA.MonitoringLocationIdentifier not MonitoringLocationIdentifier
Switch from MonitoringLocationIdentifier to TADA.MonitoringLocationIdentifier in depth profile functions, including documentation updates
Switched to using TADA.MonitoringLocationIdentifier in relevant functions
…er-in-tada_autoclean
Added TADA.MonitoringLocationName to TADA_Autoclean
Updated TADA_FindNearby to take into account org, meta data selection options, and create TADA.MonitoringLocationName
updated to use TADA.MonitoringLocationIdentifier for grouping and joining
Use TADA.MonitoringLocationIdentifier and TADA.MonitoringLocationName
Added identifier arg to TADA_OverviewMap
Updated overviewmap to include identifier param for wqp vs tada monitoring location data, added TADA.MonitoringLocationTypeName to autoclean, updated example data
TADA_GetUniqueNearbySites updates
Update TADA_FlaggedSites
Analysis filter bug fix
rearranged code related to bug fix for TADA_FindNearbySites
bug fixed - to correctly identify if no nearby sites are found
bug fix test
…er-in-tada_autoclean
…er-in-tada_autoclean
updated to include TADA_TableExport
rename CST/EPA update function in update all some example updates vignette switch to ref this branch
Bug fix for TADA_FlagDepthCategory and update some refs
problems w/ R5 data example still need to be resolved
wordlist update
updated global variables to include missing variables, mostly from mod 3 work
|
I'm making good progress on cleaning up the check issues on this branch. Once all issues are resolved, should I merge this with #568 or do you want to do any review here first, @wokenny13 and @cristinamullin? |
|
I will review later today once all the checks all good |
|
@wokenny13 - it seems like the only lingering check issue is due to the process timing out, so I think you'd be ok to go ahead and review the function updates anytime you are ready. I think fixing the timeout issue is going to involve updates of some kind to vignettes/example data like we've previously discussed. |
Great work on these! Feel free to merge them together when you are ready. |
|
I took a quick look through, this looks good to merge with #568. Maybe we can discuss in the future whether TADA_FlaggedSitesMap could use TADA.MonitoringLocationIdentifier or just MonitoringLocationIdentifier as an argument input. And likewise, on if there are use cases when any other functions that runs TADA_FindNearbySites to create TADA.MonitoringLocationIdentifier are needed or could be used as an argument input? The runtime could be a bit long from what I've seen? TADA_FindNearbySites(Data_Nutrients_UT) seems like a pretty long runtime in the example. Can a smaller example dataframe be used? Or filter this data frame down to reduce its runtime? |
|
The major disadvantage to the adjacency matrix approach now used in TADA_FindNearbySites is that it takes longer to find groups of related sites than just identifying sites within a buffer distance of each other. The main improvement that it represents is the function is no longer assigning the same site to multiple groups. So, we may need to discuss that tradeoff more as a group. If we can't get to that before the conference, I don't think that holding off on this PR would have a negative impact on any of the planned examples. |
|
I think the updates are nice, to avoid having same sites in multiple groups. However, it may be good to have a further discussion on this in one of our group meetings to think through if the longer run time is a fine tradeoff. |
Updates to TADA_FindNearby - submitting PR as I am having trouble running check() locally. I anticipate I will need to make some additional changes before it is ready for review.