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

stitch.rspec function for row-wise merging #249

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

stitch.rspec function for row-wise merging #249

wants to merge 12 commits into from

Conversation

thomased
Copy link
Collaborator

@thomased thomased commented Jun 16, 2023

Function for stitching (row-merging) rspec objects of differing wavelength ranges. It has functionality for handling specs with partly-overlapping wl regions (averaging, min, max), as well as non-overlapping wl regions (via interpolation). Can handle rspec objects when the spec names match in each but are in a different order, or when they contain only some common samples (with the other being NA filled for the relevant wl region). Edit: whoops, didn't realise it'd contain the entire commit history for #247 too.

@Bisaloo
Copy link
Collaborator

Bisaloo commented Sep 8, 2023

Function name? I'm still slightly confused on the whole method/generic thing in this case. Ideally it'd be stitch.rspec().

Here is the general gist:

  • If a generic is already defined for this in your package or one of the dependencies (including base), you just have to provide the new method as you did. This applies to function such as mean(), plot(), summary(), etc.
  • If a generic is not defined anywhere (this is your case in this PR), you have to provide the generic and the method
#' @export
stitch <- function(x1, x2, ...) {
  UseMethod()
}

#' @export
stitch.rspec <- function(x1, x2, ...) {

}

Success can be verified by the presence of a S3method() line in NAMESPACE. E.g.,

pavo/NAMESPACE

Line 12 in b1144fd

S3method(subset,colspace)

@thomased thomased requested a review from Bisaloo September 11, 2023 03:54
@thomased thomased marked this pull request as ready for review September 11, 2023 03:54
Copy link
Collaborator

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am proposing some changes here: stitch...stitch-2 to reduce special handling of edge cases. As far as I can tell, everything can be dealt with through the general case. If you agree with them, I can push them to this branch.

The implementation looks good to me. I have left one comment on design regarding argument names and whether we want to use S3 or not

Comment on lines +65 to +74
stitch <- function(rspec1, rspec2, overlap_method, interp) {
UseMethod("stitch")
}

#' @rdname stitch
#'
#' @export
stitch.rspec <- function(rspec1, rspec2,
overlap_method = c("mean", "minimum", "maximum"),
interp = TRUE) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stitch <- function(rspec1, rspec2, overlap_method, interp) {
UseMethod("stitch")
}
#' @rdname stitch
#'
#' @export
stitch.rspec <- function(rspec1, rspec2,
overlap_method = c("mean", "minimum", "maximum"),
interp = TRUE) {
stitch <- function(x1, x2, ...) {
UseMethod("stitch")
}
#' @rdname stitch
#'
#' @export
stitch.rspec <- function(x1, x2,
overlap_method = c("mean", "minimum", "maximum"),
interp = TRUE) {

To leave room for future extensions or other methods.

We may want to re-use this generic for other objects in the future and it's probably safer to offer more flexibility in the argument names. This is the same reason why we use x as the first argument in subset.rspec(), plot.rspec(), etc.

If we don't expect ever re-using this generic for other objects, we could also not leverage S3 and propose a stitch_rspec() or stitchrspec() function directly. The same way we do it with explorespec(), aggspec(), etc.

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