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

Update SoilTaxonomyDendrogram.R #45

Merged
merged 6 commits into from
Sep 16, 2022
Merged

Update SoilTaxonomyDendrogram.R #45

merged 6 commits into from
Sep 16, 2022

Conversation

brownag
Copy link
Member

@brownag brownag commented Jul 22, 2022

Here are some suggested changes to make SoilTaxonomyDendrogram() more robust to alternate data sources. If they seem reasonable these changes may be made here, and then ultimately replaced with a dedicated new function from SoilTaxonomy so that ordered factor representation can be reused in other functions.

 - default uses order, suborder, greatgroup and subgroup
 - alternately can pick just one, or a couple
 - if more than one level, last one in vector is used as labels
@brownag
Copy link
Member Author

brownag commented Jul 22, 2022

Also in a3ea312 added level argument (should it be levels? I guess I am following SoilTaxonomy package argument naming convention) to specify trees that do not necessarily use all levels of hierarchy or do not go all the way to subgroup, for instance:
image

@dylanbeaudette
Copy link
Member

dylanbeaudette commented Jul 22, 2022

OK, I like the level argument, and will modify further to not look for hard-coded elements within the SPC--that was always a messy shortcut. While this is a rather simple fix, changing column names in fetchOSD is not; will follow-up on that over in ncss-tech/soilDB#242 (comment)

I'd prefer to wait until after WCSS before merging this, unless I have a very productive weekend.

@brownag
Copy link
Member Author

brownag commented Jul 26, 2022

While this is a rather simple fix, changing column names in fetchOSD is not; will follow-up on that over in ncss-tech/soilDB#242 (comment)

Changing the default column names is not a condition of merging this PR. This PR simply adds support for users who have a SPC that has standard NASIS column names in it (and the level argument, which generally cuts down on the noisyness of larger or more diverse plots)

@brownag brownag marked this pull request as ready for review July 26, 2022 16:06
@brownag
Copy link
Member Author

brownag commented Sep 16, 2022

I think we should probably merge this and deal with any other remaining issues outside this PR.

@dylanbeaudette
Copy link
Member

Thanks for discussion. Good to merge.

 - default `level` is now a named character (default columns may change in future)
 - re-arrange some notes/messages
@brownag brownag merged commit c9a5aeb into master Sep 16, 2022
@brownag brownag deleted the dendrogramFactors branch September 16, 2022 20:07
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.

2 participants