Different semantics of groupBy and fcol

[lgatto]

fcol generally stands for a feature variable from which to extract to vector/factor, while groupBy expects a vector/factor. This lead to a problem in combineFeatures (which expects the latter) calling nFeautres (expecting the former).

Revise this behaviour, making sure that the two are supported interchangeably.

[sgibb]

Generally I like and prefer the groupBy argument because it is very flexible and don't depend on values stored in an MSnSet object. But for most users the fcol argument would be easier to understand and in most cases more than enough flexibility.

We could provide both and rewrite all groupBy/fcol functions as follows:

combineFeatures <- function(object, groupBy, fcol) {
  if (missing(groupBy)) {
    groupBy <- fData(object)[, fcol, drop=FALSE]
  }
  # ...
}

(or wrap that in a .groupBy <- function(object, groupBy, fcol) function)

Or we take the length and type of the argument: if groupBy is of length == 1 and type character treat it as fcol and otherwise as groupBy (but that would be less clear for the user and more error prone than the approach with both arguments above).

[lgatto]

I agree that groupBy is flexible, by I don't see any reason for not adding that vector/factor as a feature variable for improved traceability.

Or we take the length and type of the argument: if groupBy is of length == 1 and type character treat it as fcol and otherwise as groupBy.

That's what I had in mind. In addition, we would need to make sure that length(groupBy) == nrow(object) and fail with informative error messages if either case/conditions are not met. I thought of a function that would do all that and that we could call in combineFeature (and possibly in other functions).

I also like the other option with two arguments, groupBy and fcol. That would be a direct and good solution for combineFeatures, but I am thinking that it would be good to generalise supporting both options, and a helper function is the way to go. I would still consider two arguments to combineFeatures to harmonise the way this is done in MSnbase and pRoloc (generally through fcol rather than groupBy), but I would need review the code to make sure I am right.