obrl-soil / mpspline2

A revamped mass-preserving spline function for soils data
6 stars 2 forks source link

SPC -> MPS -> SPC wrapper? #4

Closed dylanbeaudette closed 4 years ago

dylanbeaudette commented 5 years ago

Interested in integrating the wrapper functions from here?

https://gist.github.com/dylanbeaudette/14aa4b4d43093f90f85d2379c2ae48cc

I am not partial to the names and would be happy to write docs / tests. I think that there might be a lot of interest in SPC --- MPS ---> SPC type work-flow.

If not, I'll likely add to aqp and add mpspline2 to the SUGGESTS field. Either way is OK with me.

obrl-soil commented 5 years ago

I could, but I was really not intending for this to be a permanent package. Right now, mpspline.R could be a drop-in replacement for the GSIF version if Tom, Brendan et al want to go there, and then I could just archive this.

dylanbeaudette commented 5 years ago

Well, you have done all of the work of re-implementing the code, testing, and documenting. That sounds like an R package to me. Something like this would be a fine candidate for a low-dependency set of functions that would be used by a lot of other packages.

I would be happy to assist with package maintenance if that would help.

dylanbeaudette commented 5 years ago

Also, I have re-written parts of those functions so that the original site data are preserved.

obrl-soil commented 5 years ago

Ok, I think I can go one better and give you an SPC style output from the start - check https://github.com/obrl-soil/mpspline2/commit/f4ee4f44a6f0c1d92189fdf4ae717e3b008440e7

I still had to get a bit sneaky with assigning site-level parameters thanks to the 'no duplicates' rule, unfortunately, but I took a gamble on that part of the object remaining fairly stable for now

dylanbeaudette commented 5 years ago

Thanks. I'll take a look as soon as I can. Long-term where do you see this functionality and associated documentation "existing"? I would hope that it is part of a package that finds its way to CRAN at some point.

dylanbeaudette commented 5 years ago

Thanks!

This is pretty close to the functionality as implemented in the (updated) gist.

A couple of suggestions:

I still have some TODO items in the revised gist, namely:

I would make these suggestions in a pull request but I do not want to mangle the current style of mpspline.R.

Thoughts?

obrl-soil commented 5 years ago

Hmm ok. I think I've improved the @sites slot in https://github.com/obrl-soil/mpspline2/commit/923ff077c04cc090855fca9dd6a378259f57cc2c, and I noticed that the '_icm' component should actually be the spline function estimates at the input depth ranges, not the actual input data, so that's fixed.

Otherwise, I strongly prefer to return as lean an object as possible regardless of output style. Keeping the original Site IDs (primary key) is necessary. Getting the input SID column name to persist is just a bonus, as the by = arg in both base::merge() and dplyr::*_join() can handle a naming mismatch, but I've done it anyway so it may as well stay. Not sure what you mean about the horizon depth names?

As to long term goals....undecided. I think soil science packages in general should be focused and lightweight, since our field is so broad - it lets people mix and match as they need to. At the same time, I don't think its helpful to have multiple similar implementations of the same idea floating around, and I'm aware that I'm contributing to that problem with my incessant tinkering :P

dylanbeaudette commented 5 years ago

Agreed on all points.

My suggestion: implement the leanest-meanest version of MPS in this package and return the just the essentials in a list; leave the annoying drudgery of SoilProfileCollection objects to post-processing or wrapper functions. Those functions could exist in your package (SUGGESTS / IMPORTS) or exist elsewhere (aqp, etc.).

Either way, this is great work that I would like to make more available to our staff and cite. From my perspective the mpspline2 package is preferable to the current implementation in GSIF.

pierreroudier commented 5 years ago

@obrl-soil @dylanbeaudette Thought I'd share my views (which are worth exactly 0.02 NZ$, which is not a lot).

I do think mpspline2 would be a great standalone package. The main reasons are:

obrl-soil commented 5 years ago

Thanks @pierreroudier, the more voices the better. I'm inclined to agree. Before I get too into splitting out methods and whatnot though, I want to resolve a few issues with the mathy parts of the code...

pierreroudier commented 5 years ago

@obrl-soil Great -- sing out if you want/need support in doing so. Very good idea to focus on the mathy bits, I think the main value of "packaging" mpspline2 separately is just that: getting the algorithm right and distributed in a faurly simple and flexible form.

dylanbeaudette commented 5 years ago

While I can't help all that much with the details of the algorithm, I would be happy to write, maintain, test, and document, wrapper code for current / future SPC objects.

obrl-soil commented 5 years ago

Thanks, both of you. I've taken the first step to splitting out the aqp-dependent parts of the code by moving non-default outputs into their own wrapper functions. Should then theoretically be easy to move mpspline_conv.SoilProfileCollection() and mpspline_spc() into aqp at some point.

dylanbeaudette commented 4 years ago

@obrl-soil: any plans for a CRAN release? I'm keen on integrating the wrappers into aqp in the near future. There are a lot of use cases on the horizon and we are overdue for another major aqp release. Happy to help however I can.

obrl-soil commented 4 years ago

Hey @dylanbeaudette sorry for the late reply - definitely yes, but I just have to survive another week or so of Deadline Hell first :)

brownag commented 4 years ago

@obrl-soil The work you have done here is great. There is a need for generic implementations of soil algorithms that are stable, compartmentalized and well documented.

I wanted to throw my support in for this being an independent package on CRAN as well as for the general concept of a "soil package ecosystem".

I think it is wise to encourage broader adoption of the soil-themed packages that we are not too prescriptive for "how" the analysis is implemented. The fewer hoops the user has to jump through, and the more similar the workflow can be to what they are "used to," the better, IMO.

obrl-soil commented 4 years ago

Thanks @brownag :) finally sat down and suffered through the cran submission prep stuff today. I've had to remove all the aqp-related code but it should be pretty easy to add the relevant methods and wrappers into aqp when/if this package gets on to cran. Should know in a few days...