Aqpdf #142
Aqpdf #142
Conversation
…rofileCollections (horizons, site, diagnostics, restrictions) #94
…formatting for easier use/readability
…th; in argillic application, this generally does not affect choice of illuvial horizon - but might have an effect in more aggregate data and or horizons with diffuse clay increases and/or thick transitional horizons
…obe soilDB some more using spc_in_sync)
|
Thanks for the replies. These answer most of my questions. Lets have a short conversation next time you are in the office. I'm fine with merging as long as you are confident in the testing. |
|
Please hold off on merging. I just wanted to make the pull request. It is much easier to track the progression of things in a PR. I suppose I will merge it when I am ready. |
…rk with updates to profile_compare. Just ideas for possibly eliminating cluster::daisy() dependency" This reverts commit b5c1271.
Nothing to be ashamed of, that is some very elegant code in there. Lets talk some more about it because I am about to do a total re-write of |
|
You mentioned that profile_compare needed review recently – and while I was working on various aspects of aqpdf I did spend some time looking at the comments and internals of it. I wanted to understand what it really was doing internally… playing around and documenting some ideas and doing some comparisons against profile_compare. I like to try and implement things “for myself” as a way of developing understanding. It definitely had no place in this specific branch other than it was just a little aside I did late at night. Just was trying to see if I could write something that performs similarly without dependencies. I don’t want to replace or take over profile compare – and recognize I cannot hope to possibly reimplement all the great functionality provided by `cluster` -- but that doesn’t mean I can’t learn from trying things. 😊
I genuinely thought about the major issues associated with these types of distance metrics applied to soil profile collections – most/all of which was spurred on by your code, docs and comments. We def can talk about it – this is important stuff that needs to find its way into more common use. Based on your comments, there may be a bit more going on in mine than you realize – not saying the contents are either profound or obvious.
edit: remove email reply garbage. i should never do that
|
|
The algorithm is sound, tested, applied to large collections, and applied to a diverse sets of data. That said, there are a lot of inefficiencies in the code and some style-related issues that make me want to go back and give 2009-me a whack. Most of the problems / TODO / ideas are outlined in #7 and the comments within profile_compare.R, as you found. I'm thinking about setting more default arguments ( I'd like to discuss (later this month?) your thoughts on the matter and the novelties in One small suggestion: add profile IDs and return as a Another option, try |
… have some things to get out
…zons in 3rd profile. this was also fixed in tests.
| @@ -139,7 +140,7 @@ setMethod('reorderHorizons', | |||
| order(as.character(h[[idname(object)]]), | |||
| h[[horizonDepths(object)[1]]])) | |||
|
|
|||
| h <- aqp:::.as.data.frame.aqp(h[current.order,], | |||
| h <- .as.data.frame.aqp(h[current.order,], | |||
There was a problem hiding this comment.
Is this going to be necessary for all functions that modify @horizons or slot previously containing a data.frame?
There was a problem hiding this comment.
Yes, if you want to be sure that you preserve the datatype the user set. It incurs no unnecessary overhead unless something occurred that changed the type.
|
|
||
|
|
||
| # remove bad profiles and try again: works |
There was a problem hiding this comment.
Nice catch. I'll review the current logic, wouldn't be surprised if it has missed something.
| @@ -169,4 +169,6 @@ test_that("addVolumeFraction fractional horizon depths", { | |||
|
|
|||
| }) | |||
|
|
|||
|
|
|||
| # cleanup | |||
| if (file.exists("Rplots.pdf")) | |||
|
Documentation (#139) is implemented for all of the fundamental internal S4 functions for the SPC class that manipulate slots etc. The R CMD check manual page problems have been fixed -- there were a couple mistakes I had made -- typos in names, missing things, duplications etc. I will probably need to write a Wiki page to make it easier / remember all the details for the future and make sure everyone knows how to do it. I will be moving on to others (plots, slice/slab, profile_compare, etc) in the near future now that I have ironed out the process. roxygen is very straightforward with ordinary functions but the S4 methods require a lot more care. With S4, I think some of the things I am be doing are redundant relative to most modern version of roxygen and its ability to "guess" the right values -- so with some more testing I can probably pare down the markup further. EDIT: already informed of issue with |
Merge branch 'master' into aqpdf # Conflicts: # man/profile_compare-methods.Rd
add support for data.table and tibble