Skip to content
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

Update TADA_FindNearbySites flag column TADA_MonitoringLocationID #292

Open
cristinamullin opened this issue Jun 16, 2023 · 5 comments
Open
Labels
Future Improvement Minimum viable function complete, issue includes potential future improvements Module 1 MVP ResultFlagsIndependent.R Top Priority

Comments

@cristinamullin
Copy link
Collaborator

See remaining discussion points and questions here: #283

@cristinamullin cristinamullin added ResultFlagsIndependent.R Future Improvement Minimum viable function complete, issue includes potential future improvements More Research Needed labels Jun 16, 2023
@ehinman
Copy link
Contributor

ehinman commented Aug 3, 2023

Copying comments from previous PR so easier to find for this issue:

  • discuss automatically updating TADA.MonitoringLocationIdentifier to the new MonitoringLocationIdentifier for the grouping, if user desires to assess the grouping as a "single" site
  • remaining discussion points/tasks related to this PR:
  • Discuss how we want to display this information and how we want users to be able to navigate these duplicates. Include additional information in documentation and in Module 1 vignette
  • Discuss scenario where a user desires to assess the grouping as a "single" site. Should we: A) automatically update the TADA.MonitoringLocationIdentifier to a new MonitoringLocationIdentifier generated for the grouping to facilitate this, B) allow the user to choose one of the two "duplicated" MonitoringLocationIdentifier's, or C) help user input a new name.

@cristinamullin
Copy link
Collaborator Author

cristinamullin commented Feb 12, 2024

Update:

Default behavior:
Suggest to rename the TADA_FindNearbySites column that is generated as TADA_MonitoringLocationID in preparation for Module 2. This new column should be used in mods 2 and 3. It can use the concatenated IDs as the default. Still need to consider how a user might select one of the two IDs to use instead, or generate a new ID.

@cristinamullin
Copy link
Collaborator Author

cristinamullin commented May 23, 2024

On our 5/23/24 call we decided to use the json array to concatenate the monitoringlocationID's to be consistent with the way the WQX 3.0 profiles will be serving 1-to-manys in the new profiles (coming later this year).

Example:
["Lakes","RLSWQD","Small Lakes"]

I created a separate issue for this: #463

@cristinamullin
Copy link
Collaborator Author

@renaemyers all other topics related to this issue are now addressed elsewhere. This issue can be closed once your edits to TADA_FindNearbySites are merged, with the concatenation format you already showed today for the IDs. Any edits to the formatting (JSON array) and dev of the new function for parsing it can wait until the WQX 3.0 profiles are more finalized: #463

@cristinamullin cristinamullin changed the title Duplicate results and nearby sites data flags Update TADA_FindNearbySites flag column TADA_MonitoringLocationID Jun 4, 2024
@cristinamullin
Copy link
Collaborator Author

@renaemyers

The current TADA.NearbySiteGroups column is being replaced by a new different column TADA_MonitoringLocationID in this issue.

Whenever we change existing functions in the TADA package, we need to review the package and R shiny app to see if and where else (within what other functions) the edited function is used and update those as well if needed so they don't break/bug.

A quick search of the package (click edit tab at top of R studio console, click "Find in Files", and search for all references to TADA.NearbySiteGroups column in package and/or references to the TADA_FindNearbySites function) shows that TADA.NearbySiteGroups is leveraged in:

  • TADA_FindPotentialDuplicatesMultipleOrgs

This function should be updated and tested to use the new column (TADA_MonitoringLocationID) instead of the old (TADA.NearbySiteGroups), and to ensure it works with the update made as as part of this issue.

The shiny app does not include the TADA_FindNearbySites function yet so no updates needed there.

You'll notice that TADA_CalculateTotalNP in Transformations.R mentions the TADA_FindNearbySites function (lines 233 and 319) but it is not directly used. This function should be updated as well but it can happen as part of a separate PR/issue because it will not cause any breaking changes. See related issue: #475

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Future Improvement Minimum viable function complete, issue includes potential future improvements Module 1 MVP ResultFlagsIndependent.R Top Priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants