-
Notifications
You must be signed in to change notification settings - Fork 19
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
TADA_CreateParamRef() and TADA_CreateParamUseRef() #555
base: develop
Are you sure you want to change the base?
Conversation
First 2 reference files draft functions have been pushed through.
working on addressing some check issues that were found |
TADA_CreateParamRef and TADA_CreateParamUseRef .RD files
removal of some intermediate objects
Update - only print df if there are failing URLs
documentation suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments about requested changes are in-line or discussed in our working session call.
R/CriteriaInputs.R
Outdated
if (n > 100) { | ||
message(paste0("There are ", n, " unique TADA.ComparableDataIdentifier names in your TADA data frame. | ||
This may result in slow runtime for TADA_CreateParamRef() if you are generating an excel spreadsheet. | ||
Consider filtering your .data TADA dataframe to a smaller subset of TADA.ComparableDataIdentifier first.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might belong in documentation instead of in this message, but maybe some suggestions on how to create smaller subset of TADA.ComparableDataIdentifier would be helpful for users. For example, suggest working through characteristicType groups when setting up these ref files for the first time.
R/CriteriaInputs.R
Outdated
#' with the corresponding unique list of TADA/WQP Characteristic Names/TADA.ComparableDataIdentifier. | ||
#' | ||
#' If an ATTAINS parameter name is not listed as a prior domain value for your org from prior | ||
#' ATTAINS assessment cycles, users should consider contacting the ATTAINS team to add this to the domain list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. The ATTAINS team can't add a parameter to an org. Only the org can do that by associating that parameter with a particular assessment unit/designated use. Any org can use any parameter in ATTAINS (even if it was requested by a different org). Users should only contact the ATTAINS team to add a parameter name if there is not an acceptable/appropriate parameter name in ATTAINS.
As far as ATTAINS is concerned, there is no issue with an org using any ATTAINS-accepted parameter for the first time. So the ATTAINS team does not need to do anything in this situation.
R/CriteriaInputs.R
Outdated
#' Otherwise, users can still proceed by overriding the data validation by value pasting. | ||
#' Users will be warned in the ATTAINS.FlagParameterName column if they choose to include an | ||
#' ATTAINS.ParameterName that was not named in prior ATTAINS assessment cycles as: | ||
#' 'Suspect: parameter name is not found as a prior parameter name for this organization' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a way to flag differentiate between "This is an accepted ATTAINS parameter, that has not been previously used by this organization" and "This is not an accepted ATTAINS parameter, contact the ATTAINS team to inquire about adding it" as from an ATTAINS perspective these are two very different scenarios.
Is there still an option for the validation in Excel to allow ALL ATTAINS parameters, not just the ones from a specific org? This may be an important option for users, especially for orgs (tribes? territories?) that in the process of growing their assessment programs and may not yet have many assessments completed. In these cases, they might rather be able to access the whole possible list of parameters from ATTAINS rather than be limited to the handful that may have been listed as causes for a small number of assessments.
They may also find that newer parameter names may be more appropriate than ones that have been used in the past if their have been changes to assessment methodologies or more specific parameter names have been added (for example: specific aquatic plant species vs. more general plant parameter or a specific cyanotoxin vs. a more general algal parameter). So while I think using previous causes as a starting point for the parameter crosswalk is helpful and makes the process less overwhelming, we shouldn't make it too difficult for users to apply other ATTAINS parameter names to their crosswalks if they want or need to.
#' | ||
#' If an ATTAINS parameter name is not listed as a prior domain value for your org from prior | ||
#' ATTAINS assessment cycles, users should consider contacting the ATTAINS team to add this to the domain list. | ||
#' Otherwise, users can still proceed by overriding the data validation by value pasting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the only way to override the data validation is by pasting (not just typing in a different entry) could we provide a sheet in the Excel file that lists all the current ATTAINS parameter names (without filtering them by organization)?
R/CriteriaInputs.R
Outdated
#' if there is trouble locating the file. The file will be named "myfileRef.xlsx". | ||
#' The excel spreadsheet will highlight the cells in which users should input information. Users may need to | ||
#' insert additional rows to capture certain ATTAINS.ParameterName that correspond with multiple TADA.CharacteristicName. | ||
#' Example: If your organization defines pH as "pH, High" and "pH, Low" that correspond to the TADA.CharacteristicName 'PH'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an example of the opposite scenario than initially described as this is multiple ATTAINS parameter names to one TADA.CharacteristicName
#' # After running TADA_CreateParamRef() users will provide this as a function input. First example is only for the org Utah | ||
#' paramUseRef_UT <- TADA_CreateParamUseRef(Data_Nutrients_UT, paramRef = paramRef_UT3, org_names = c("Utah"), excel = FALSE) | ||
#' | ||
#' # Users can include the EPA304a standards by itself or compared to their org(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this example to show both org/304a and just 304a - really useful
R/CriteriaInputs.R
Outdated
} | ||
|
||
if (is.null(org_names)) { | ||
print("No organization name provided, users should provide a list of ATTAINS domain organization state or tribal name that pertains to their dataframe. Attempting to pull in organization names found in the TADA data frame.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistencies in printed message format. List function name first or not?
R/CriteriaInputs.R
Outdated
} | ||
|
||
if (sum(!is.na(paramRef$ATTAINS.ParameterName)) == 0) { | ||
stop("No values were found in ATTAINS.ParameterName. Please ensure that you have inputted all field values of interest in the ATTAINS.ParameterName column generated from TADA_CreateParamRef() function") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State more explicitly that the function cannot continue without some ATTAINS.ParameterName inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might want to change this to a warning. I am considering the case in which a user is only interested in the EPA304a standards. In this scenario, there is not a need for a user to provide a crosswalk between ATTAINS.ParameterName and TADA.ComparableDataIdentifier as it will only leverage the EPA304aPollutantName.
} | ||
|
||
if (sum(is.na(paramRef$ATTAINS.ParameterName)) > 1) { | ||
print("NAs were found in ATTAINS.ParameterName. Please ensure that you have inputted all field values of interest in the ATTAINS.ParameterName column generated from TADA_CreateParamRef() function") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to NA could a "No corresponding ATTAINS.ParameterName" option be added to the ATTAINS.ParameterName drop-down list? Then in situations where a user has thoroughly reviewed the param list, they would not get this message if they had reviewed and provided some input for each entry in the ATTAINS.ParameterName field
R/CriteriaInputs.R
Outdated
} | ||
|
||
if (sum(is.na(paramRef$EPA304A.PollutantName)) > 1 && org_names == "EPA304a") { | ||
print("NAs were found in EPA304A.PollutantName. Please ensure that you have inputted all field values of interest in the EPA304A.PollutantName column generated from TADA_CreateParamRef() function if you are interested in using the 304a recommended standards") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do users input this? Or is it done automatically? or a combination? Might be worth clarifying in the message and documentation that not all 304a pollutant names can be automatically crosswalked, so user review/input is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. I had these lines included in past development to consider allowing users to also make edits to the EPA304a.PollutantName. But was unsure if we wanted to give this option to allow these values to be changed?
Making it clear that 'not all 304a pollutant names can be automatically crosswalked, so user review/input is required' will be nice to include if we want to proceed with allowing users to make further edits to the efforts we made with this crosswalk if an org feels one is more appropriate.
Added a tab for org_name filtered paramter and use names from ATTAINS Modified return values of ATTAINS.FlagParameterName
@cristinamullin @hillarymarler There are still some edits and formatting I would like to make, but I think this is in a good spot to review now. I will be out of the office tomorrow but will be back on Friday the 20th. |
R/TADARefTables.R
Outdated
#' TADA_CreateParamUseRef() as the basis for the pulling in prior ATTAINS | ||
#' parameter names and use name by organization name. This helps to filter | ||
#' selections of drop down values and creating parameter and use combination | ||
#' summaries that will need to be defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could remove "that will need to be defined"
R/TADARefTables.R
Outdated
#' ATTAINS Parameter and Use Name by Organization Reference Key | ||
#' | ||
#' Function downloads and returns the newest available ATTAINS domain values | ||
#' reference dataframe summarized by parameter, use and organization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add clarifying language about parameters (listed as cause by org in previous assessments)
R/TADARefTables.R
Outdated
#' TADA_CreateParamUseRef() as the basis for the pulling in EPA304a recommended | ||
#' pollutant name and use_name for assessment under the CWA. | ||
#' | ||
#' (Currently only numeric priority characteristic in TADA are the focus.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add info on where to find list of TADA priority chars?
…/USEPA/EPATADA into Create-Module-3-Reference-Tables
Cleaned the updating process for this ref table. Will need to make further edits to reflect changes throughout .R function dependencies.
Reflects some updates with the CST ref table. CreateParamUseRef() now contains a column for Include or Exclude.
Added a column to the CST file.
Provided additional details and formatting fixes.
Mod 3 notes, additional comments, and a minor fix in removing duplicate rows for dataframe output in CreateParamRef()
CreateAUIDRef() is in development and was not meant to be included in the prior commit
First 2 reference files draft functions have been pushed through to test.
I am still working on the Mod 3 vignettes, however the R document contains a detailed explanation of what the two functions' goals are, and can be reviewed in the meantime while I continue to work on the Mod 3 vignettes.