bin: y argument must be an ordered vector

[RogerGinBer]

Hi there!
I was looking into the function bin for calculating ion-mobility mobilograms (sneumann/xcms#647) and, while experimenting, it seems that the argument y must be an increasingly ordered vector (otherwise it creates a silent bug). I would suggest adding a check for ordered data and/or documenting this requirement in the docs.

I'm guessing that for its original intended purpose (binning mz values) this was not an issue, as mz values are always ordered in a Spectra object.

Here's a reproducible example of the problem:

 xvec <- 1:20
 yvec <- c(41:50, 1:10)
 res_bad <- MsCoreUtils::bin(x = xvec, y=yvec)  ## Incorrect result
 res_good <- MsCoreUtils::bin(x = xvec[order(yvec)], y=sort(yvec)) ## Correct result
 plot(yvec, xvec)
 plot(res_bad$mids, res_bad$x)
 plot(res_good$mids, res_good$x)

## Vector relative ordering shouldn't matter when binning, yet in here the output$x is reversed
identical(MsCoreUtils::bin(x = 1:10, y=1:10),
               MsCoreUtils::bin(x = rev(1:10), y=rev(1:10))) ## FALSE, but it should be TRUE

[jorainer]

Great catch @RogerGinBer ! May I suggest you make a PR (which includes a unit test) for this?

[sgibb]

@RogerGinBer thanks for the catch and the report. Unfortunately I don't believe the solution is as easy as simply reordering x and y.

bin(1:3, 1:3) should only be identical to bin(3:1, 3:1) if the breaks are identical (which is TRUE for the default argument).

bin(1:3, 1:3, breaks = 1:3) should be different from bin(1:3, 1:3, breaks = 3:1) (which currently don't work).

I am wondering if we should allow unsorted inputs at all. Your current PR #110 silently reorders x and y which could yield unexpected errors because as user you could expect that bin(1:3, 1:3) == rev(bin(1:3, rev(1:3))).

Currently I am thinking that we should check that y and breaks are sorted and throw an error if not. @jorainer maybe we should introduce a .check argument to skip these (as we did in closest) to not slow down the function in Spectra methods.

[RogerGinBer]

Hi there @sgibb! I see, I knew I was probably missing something... 😅 I never tried setting breaks to a value different than the default.
From my testing, breaks's order is already being checked by findInterval, so it must always be increasingly sorted for that function to work. Thus, bin(1:3, 1:3, breaks = 3:1) could never work (unless we sort it first, run the binning and reorder it at the end, but that's probably another story).

So, I currently see these options:

• To silently reorder x, y (and breaks)
• To throw a warning to let the user know and reorder them anyway
• To throw an error if either y and/or breaks are not ordered

Personally, I'd probably go with the second option, since it's a situation from which the function can recover and still give a valid output, but the user must know that something was amiss. I also see your point in not allowing unsorted inputs at all, so that devs that use the function must consciously take care of ordering their inputs.

I agree that a .check argument would be a good way to avoid slowing down the function if you are sure the arguments are already well ordered, so I can add that to the PR if we agree on reordering. Silly question, would the default be TRUE or FALSE?

[lgatto]

Currently I am thinking that we should check that y and breaks are sorted and throw an error if not.

This also sounds reasonable to me, as I think this is what users expect.

[jorainer]

Sorry for being late. After having some thoughts I agree with @lgatto and @sgibb . We should not silently order x but throw an error if x is not sorted. I don't think there is any need to check breaks since, as you said, findInterval will check that anyway.

The .check parameter should be .check = TRUE by default. That way we can call this function with .check = FALSE on a Spectra object where we require m/z values to be sorted.

[RogerGinBer]

Good! Since we all agree, l'll update PR #110 accordingly (add .check parameter, throw error if y is unordered, remove silent ordering) 👍

[jorainer]

Closing as the PR is now merged.