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

Addition of function force_sorted() #123

Merged
merged 11 commits into from
Mar 21, 2024
Merged

Addition of function force_sorted() #123

merged 11 commits into from
Mar 21, 2024

Conversation

philouail
Copy link
Collaborator

Here I propose the function force_sorted() which takes as an input a vector of numeric values and interpolate the values that are not increasing.

I wrote this function specifically for a retention alignment method that is being developed in xcms and at some point i had to deal with unsorted/non-increasing retention times values. However, I believe it could be used in the future or by user for some other reasons.

Also, would welcome inputs on the name, as the function do no sort() the values per se, but we do check for the results by checking is.unsorted().

I also thought about a possible extra parameter keep.na = TRUE which would (if FALSE) allow the user to decide if they want the NA values to be adjusted too or not.

Welcoming feedback :)

R/force_sorted.R Outdated Show resolved Hide resolved
R/force_sorted.R Outdated Show resolved Hide resolved
R/force_sorted.R Show resolved Hide resolved
R/force_sorted.R Outdated Show resolved Hide resolved
Copy link
Member

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Thanks Phili for the contribution. I'm generally OK, only some minor fixes in the documentation.

NEWS.md Outdated Show resolved Hide resolved
R/force_sorted.R Outdated Show resolved Hide resolved
R/force_sorted.R Outdated Show resolved Hide resolved
R/force_sorted.R Outdated Show resolved Hide resolved
R/force_sorted.R Outdated Show resolved Hide resolved
src/MsCoreUtils.dll Outdated Show resolved Hide resolved
@jorainer
Copy link
Member

Maybe also to give a bit more of a context to this function: in the alignment (retention time adjustment) of LC-MS data we sometimes observe that the order of adjusted retention times of spectra is no longer ordered - which in fact should not happen, because spectra should always be ordered as they were acquired in the LC-MS run - and this is ordered by retention time. So, in cases were some spectra have out-of-order adjusted retention times we want to force them into order again.

At present, this code is used in xcms, but will then eventually also be needed in the Chromatograms package - hence adding it here to MsCoreUtils.

@philouail philouail requested a review from lgatto March 20, 2024 07:45
R/force_sorted.R Outdated
#'
#' ## We can see the values were not interpolated but rather replaced by the
#' ## last increasing value `2` and increasing by 0.1.
#' sorted_vec
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be sorted_rtime. Also could you use another name, like x2 or y, to keep it general. Here, I guess rtime refers to retention time, but this isn't mentioned in the man page (and there's no need to), so it is a bit of an obscure name.

R/force_sorted.R Outdated
warning("Found decreasing values at the end of the vector. ",
"Interpolation is not possible in this region. Instead, ",
"replacing these values with a sequence that starts from ",
"the last increasing value and increments by ", by,
Copy link
Member

Choose a reason for hiding this comment

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

There are two spaces after by.

@lgatto
Copy link
Member

lgatto commented Mar 20, 2024

Thank you for the contribution!

Copy link
Member

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Thanks Phili! green lights also from my side. Will merge and push to BioC.

Thanks!

@jorainer jorainer merged commit f9ead20 into main Mar 21, 2024
4 of 6 checks passed
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.

3 participants