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

removal/combination of restrictions slot #167

Open
brownag opened this issue Sep 18, 2020 · 2 comments
Open

removal/combination of restrictions slot #167

brownag opened this issue Sep 18, 2020 · 2 comments

Comments

@brownag
Copy link
Member

brownag commented Sep 18, 2020

{aqp} SoilProfileCollections are supposed to be schema-agnostic. In general the @diagnostic slot has been used for diagnostic features. A while back, I added a @restrictions slot for the related (but different in SSURGO/NASIS) restrictions table.

At the time, it seemed simpler to add a slot rather than disrupt the current workflows where @diagnostic contains only diagnostic features.

I propose that the restrictions slot be removed and common use cases of these slots (e.g. by soilDB fetch functions) be updated to add both restrictions and diagnostics to the diagnostics slot. {aqp} should provide several methods to facilitate this. In principle, one should be able to add diagnostics and restrictions, preserving original column names by data.table::rbindlist(fill = TRUE) creating "wide" tables that should, in general, be backwards compatible with existing code. {aqp} would continue to define diagnostic_hz getter/setter, whereas the restrictions methods that are currently less frequently used could be defined as a SoilProfileCollection-method within {soilDB} (that uses something like, pattern matching on column name to find "restr", or makes use of origin metadata to match schema)

I have been meaning to put this into an issue for some time. I guess I want feedback on two things.

  1. Should the restrictions slot be removed from the SoilProfileCollection?

  2. If restrictions slot is removed, how can we best retain the existing functionality and also expand to deal with more generalized zones (control sections, etc) that might be valuable to visualize with addDiagnosticBracket?

@brownag
Copy link
Member Author

brownag commented Sep 18, 2020

Tagging @smroecker @jskovlin in particular for thoughts on this.

I didn't seek enough feedback prior to implementing this change so I'd like to at least fix it in a way that is mutually agreeable.

@brownag
Copy link
Member Author

brownag commented Sep 22, 2020

I was thinking about this some more and was wondering if this could be an opportunity to generalize diagnostic_hz as well as restrictions. We could define a SPC method such as feature/feature<- that handles arbitrary data.frame-like data containing top and bottom depths of zones that have some sort of group and unique label.

e.g.

feature(spc, type = "diag") <- diagnostic.data.frame
feature(spc, type = "restr") <- restr.data.frame
feature(spc, type = "psc")   <- psc.data.frame
# get just diagnostic feature types
# ... filter columns with "diag" in name? and rows with type "diag"?
feature(spc, type = "diag")

# get all feature types
feature(spc)

Instead of removing diagnostic_hz/restrictions and their setters, redefine them to use the feature method.

diagnostic_hz <- function(spc) {
  feature(spc, "diag") 
}

I don't think this is ideal --but just another idea. The above provides a way to keep definitions of diagnostic_hz and restrictions in {aqp} while further generalizing the single data.frame-like slot for other types of soil depth intervals.

addDiagnosticBracket has esentially all of the relevant arguments to support the more generalized result. Not sure if it could be further streamlined.

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

No branches or pull requests

1 participant