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_chorizon_from_SDA: concatenate many:1 texcl, lieutex within RV chtexturegrp #353

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

brownag
Copy link
Member

@brownag brownag commented May 17, 2024

This PR will resolve at least one case that can lead to issues like #348.

We already limit horizon texture group information for get_chorizon_from_SDA() to just representative, but about 2% of the time the RV group has multiple chtexture records and multiple texcl or lieutex associated with it. At least for the case of stratified textures this is permissible.

This PR concatenates texcl and lieutex columns in the result with a comma to make the result 1:1 with the RV chtexturegrp record. Thus it will be less likely to artificially duplicate horizons / cause some profiles in result SPC to be invalid.

In general, it is not correct to have multiple RVs marked--the point of the RV is to provide a single representative record. Component horizons with multiple RV texture groups (not "allowed" but does exist in rare cases) are not handled by this new aggregation step and still will result in duplication--as is the case with most cases where multiple RVs are marked.

This change means that when running fetchSDA() or similar with NASISDomainsAsFactor(TRUE) NA values will result for records that have concatenated texcl, rather than one of the 21 factor levels specified in aqp::SoilTextureLevels().

While this is a change in behavior, these days the automatic factor level setting is not default and probably not that widely used. My preference is that we empower users to assign logical factor settings to their data themselves, and deal with any missing/duplicate values prior to that. This may mean a vignette covering get_NASIS_column_metadata(), NASISChoiceList(), uncode() and similar methods may be worth developing.

@brownag brownag marked this pull request as ready for review May 30, 2024 15:02
@brownag brownag linked an issue Jun 10, 2024 that may be closed by this pull request
@brownag brownag merged commit 41f86ec into master Jun 10, 2024
5 checks passed
@brownag brownag deleted the sda-chorizon1 branch September 17, 2024 21:06
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

Successfully merging this pull request may close these issues.

get_chorizon_from_SDA() returning duplicate horizons
1 participant