-
Notifications
You must be signed in to change notification settings - Fork 18
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 to WQX ref (ft, in, m) and TADA (mass/mass to mass/volume) ref #522
Update to WQX ref (ft, in, m) and TADA (mass/mass to mass/volume) ref #522
Conversation
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.
Please add conversions to ug/L and mg/L for other mass/mass units. Once you've added those, I will approve this PR as it is working well, just needs a few additions. Nice work!
ug/kg,8/29/2024 12:00,mg/L,1.00E-03,0 | ||
#/100mL,8/29/2024 12:00,CFU/100ML,1.00E+00,0 | ||
%,8/29/2024 12:00,ug/L,1.00E-03,0 | ||
%,8/29/2024 12:00,mg/L,1.00E-06,0 |
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 test data sets, I am still seeing a number of mass/mass units that are not converting because conversion factors to common mass/volume units are not included in TADAPriorityCharConvertRef.
For example: ug/g, mg/kg, ng/g, ng/kg
I think looking at the "Concentration Percentage" units in the WQX MeasureUnit.csv can help generate a list of the missing ones. It would be great to include conversion factors for ug/L and mg/L (as you already have for fg/kg, g/g, ug/kg, etc.)
- Updates reference tables and example data - Removes TADA.WQXTargetUnit and adds TADA.Target.ResultMeasure.MeasureUnitCode to required cols - Update ordering of columns and comparable data ID when transform = FALSE - Adds TADA.WQXUnitConversionCoefficient and TADA.WQXResultUnitConversion to column ordering (only relevant when transform = FALSE) - Update documentation
- Allows function to run without autoclean being run first - found that PPM was "Accepted" unit for "Depth, Bottom". Changed this to "Rejected" in WQX validation table. Reviewed many other depth result units as well as part of this update. - Updated example data
…-massvolume---kw' into fix-unit-conversions
@cristinamullin - the reason I had left the units in the TADA_CreateUnitRef output was for cases where a user might want to convert to a unit that we don't have in the unit refs. But if that scenario is not going to happen (or extremely unlikely), then I agree we can remove those columns as users can access the conversion info through TADA_ConvertResultUnits when transform = FALSE. @wokenny13 - do you want to work on the updates needed for TADA_CreateUnitRef? Or would you like me to do that? |
@hillarymarler I can work on making these updates. I will let you know if I have any questions as I catch up on these comments. |
Additional rows for other mass/mass to mass/volume Removed the two columns for TADA.WQXUnitConversionFactor and TADA.WQXUnitConversionCoefficient in the function TADA_CreateUnitRef
Changed order of columns to make it more clear on which columns are using the conversion factor
The test_that should re-run the TADA_RandomTestingData pull up to 10 times for testdat1. It will stop and determine if dimensions are the same. if testdat2 is able to run without error, that is if paired parameters in the testdat1 are in its dataframe.
updated comments and styler
@hillarymarler @cristinamullin
|
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.
@wokenny13 - the changes you made look good! I like the changes to the CriteriaComparison test.
@cristinamullin - did you want to take another look at any of this PR?
These updates look great - thank you @wokenny13! I just merged in updates from develop. Once the checks complete, it can be merged in. |
used break to break the loop. defined testdat as a global variable list that contains the two dataframes in the trycatch error function of testdat1 and testdat2
Kevin has updated the WQX ref to reflect the changes. We decided to keep target units to inches, and fixing those 3 rows for the conversion factors.
I've added additional rows in TADA.PriorityCharConvertRef for common mass/mass to mass/volume conversions.
I ran a random testing TADA_DataRetrieval and found a #/100mL unit with no conversions to CFU/100mL, and added on this as a row to TADA.PriorityCharConvertRef in the csv.
Checking the conversion factors for reasonableness will be good for review, and if these additional rows make sense.