-
Notifications
You must be signed in to change notification settings - Fork 13
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
sieve()
for classifying arbitrary coarse fractions
#283
Conversation
Nice, thanks for implementing. I like the more generalized approach in this version. I've made a couple of tiny adjustments. A tiny change, but what do you think about "class_" as the generic name vs. "grp" ? |
Totally fine with that! I pondered the default value there for about 0.5 seconds after realizing the code didnt work if |
Thanks for the additional example! Also for docs probably this needs a reference to the SSM, field book, or similar for the (default) size classes. |
OK, thanks. Good ideas. I'll adjust default prefix, and a citation. Soil Science Division Staff. 2017. Soil survey manual. C. Ditzler, K. Scheffe, and H.C. Monger (eds.). USDA Handbook 18. Government Printing Office, Washington, D.C. |
A couple other things I noticed:
|
Good ideas. Feel free to add notes / examples related to artifacts. |
Another idea, but I don't have a use case: optionally returned ordered factors. |
This briefly occurred to me too; possibly a function like |
still filling out documentation
I like that idea. Now ~90% implemented via |
Have a peek, I'm done here and happy with the results. |
This is following up from a soilDB issue ncss-tech/soilDB#217 where we wanted to make the workhorse sieve function (with custom thresholds) available for more general usage outside soilDB.
The function defined by this PR should be adequate as is to replace the internal soilDB
.sieve()
method (sometime in the future, probably after adding a aqp >2.0 import req). In the meantime we can do whatever we want to improve what this one does, and then adjust soilDB accordingly.