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

get_SDA_property(method = 'MAX') oddity #219

Closed
dylanbeaudette opened this issue Nov 10, 2021 · 1 comment · Fixed by #220
Closed

get_SDA_property(method = 'MAX') oddity #219

dylanbeaudette opened this issue Nov 10, 2021 · 1 comment · Fixed by #220
Labels

Comments

@dylanbeaudette
Copy link
Member

Noticing something strange when using MAX aggregation.

library(soilDB)

mu <- '623426'

# note: one of these is a minor component
SDA_query("SELECT mukey, cokey, compname, majcompflag from component where mukey = '623426';")


# ok
get_SDA_property(property = 'claytotal_r', top_depth = 0, bottom_depth = 25, method = 'None', mukeys = mu, include_minors = TRUE)

# ok
get_SDA_property(property = 'claytotal_r', top_depth = 0, bottom_depth = 25, method = 'Dominant Component (Numeric)', mukeys = mu, include_minors = TRUE)

# ok
get_SDA_property(property = 'claytotal_r', top_depth = 0, bottom_depth = 25, method = 'Weighted Average', mukeys = mu, include_minors = TRUE)

# ok
get_SDA_property(property = 'claytotal_r', top_depth = 0, bottom_depth = 25, method = 'MIN', mukeys = mu, include_minors = TRUE)

# not right, should be 45
get_SDA_property(property = 'claytotal_r', top_depth = 0, bottom_depth = 25, method = 'MAX', mukeys = mu, include_minors = TRUE)
@brownag
Copy link
Member

brownag commented Nov 10, 2021

Thanks for reporting this. Three things here (that I can fix/upgrade)

  • top_depth and bottom_depth are currently used only for method="weighted average" and "dominant component (numeric)" -- this is documented but unexpected/easy to forget -- so the correct value is 47.5 not 45 (as written). It is still wrong because it returns the MAX for the dominant component in this case

  • Likewise include_minors currently has no effect on the MIN/MAX aggregation -- minors are always included (as written)

  • The second subquery for MIN/MAX orders on comppct_r and takes the first record which appears to lead to this inconsistency -- this is easy to fix for single-property queries but becomes very complicated with vectorization. Need to think on this a bit and re-engineer. Will open a PR. For instance in a query calling MIN on claytotal_r and sandtotal_r in your example the former would come from the Braman component and the latter from Osage.

brownag added a commit that referenced this issue Nov 11, 2021
…, top/bottom depth, and needs refactoring

 - Fix for #219
@brownag brownag added the bug label Nov 11, 2021
brownag added a commit that referenced this issue Nov 12, 2021
- does not support include_minors, top/bottom depth, and needs refactoring
- Fix for #219
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.

2 participants