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

ft, m, in unit conversions not working #517

Open
8 tasks
cristinamullin opened this issue Aug 26, 2024 · 4 comments · May be fixed by #522
Open
8 tasks

ft, m, in unit conversions not working #517

cristinamullin opened this issue Aug 26, 2024 · 4 comments · May be fixed by #522
Labels

Comments

@cristinamullin
Copy link
Collaborator

Describe the bug

TADA_ConvertResultUnits was not converting correctly from m, ft, or in to target of in. Ideally, the target for all of these would be m. I am not sure where the IN target or conversion factors are coming from.

I think this is coming from the WQX unit domain, but what I am seeing in WQXweb vs. the domain table (https://cdx.epa.gov/wqx/download/DomainValues/MeasureUnit.CSV) vs. the TADA WQXunitref are not consistent. Need more time to troubleshoot.

To Reproduce

Code to reproduce the behavior:

test <- TADA_DataRetrieval(
  startDate = "2020-01-01",
  endDate = "null",
  countrycode = "null",
  countycode = "null",
  huc = "null",
  siteid = "null",
  siteType = "null",
  characteristicName = "Depth, bottom",
  characteristicType = "null",
  sampleMedia = "null",
  statecode = "null",
  organization = "null",
  project = "null",
  providers = "null",
  applyautoclean = FALSE
)

test2 <- TADA_CreateUnitRef(test)

test3 <- TADA_ConvertResultUnits(test)

Screenshots

image **Additional context**

Bug fixes should include all the following work:

  • Create or edit the function/code.

  • Document all code using line/inline and/or multi-line/block comments
    to describe what is does.

  • Create or edit tests in tests/testthat folder to help prevent and/or
    troubleshoot potential future issues.

  • Create or edit the function documentation. Include working
    examples.

  • Update or add the new functionality to the appropriate vignette
    (or create new one).

  • If function/code edits made as part of this issue impact other
    functions in the package or functionality in the shiny app, ensure
    those are updated as well.

  • Run TADA_UpdateAllRefs(), TADA_UpdateExampleData(), styler::style_pkg(),
    devtools::document(), and devtools::check() and address any new notes or
    issues before creating a pull request.

  • Run more robust check for releases: devtools::check(manual = TRUE,
    remote = TRUE, incoming = TRUE)

@hillarymarler
Copy link
Collaborator

I am wrapping up some unit conversion work related to ref = "wqx". Do you want me to take a look at the length unit conversions too?

@wokenny13
Copy link
Collaborator

wokenny13 commented Aug 27, 2024

The TADA.WQXUnitConversionFactor are all 0.0254 in the screenshot example that Cristina shared, which doesn't seem right.

I am looking at the WQXUnitRef.csv and it seems like that is where that 0.0254 is coming from? Also, the last changed date was recent at 3/26/2024.

image

Should the target.unit be in 'm'? Should this be brought up to WQX/WQP team at all? There doesn't seem to be a 'm' Target.Unit in WQXUnitRef at all, only in 'in'.

@wokenny13
Copy link
Collaborator

image

Should these 3 rows in the WQXUnitRef be corrected to the values shown in the last column here?

@wokenny13
Copy link
Collaborator

wokenny13 commented Aug 27, 2024

image

Should these 3 rows in the WQXUnitRef be corrected to the values shown in the last column here?

I believe these conversion factors would be correct if the Target.Unit for these 3 rows were changed to 'm'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@cristinamullin @hillarymarler @wokenny13 and others