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

feat: add new filterPeaksRanges function #326

Merged
merged 3 commits into from
Jul 19, 2024
Merged

feat: add new filterPeaksRanges function #326

merged 3 commits into from
Jul 19, 2024

Conversation

jorainer
Copy link
Member

  • Add a new filterPeaksRanges function that allows to filter mass peaks in a Spectra by any (combination of) numeric spectra and peaks variables.
  • Re-organize documentation of filter and subset operations (related to issue Split Spectra documentation into smaller chunks #288).

- Add a new `filterPeaksRanges` function that allows to filter mass peaks in a
  `Spectra` by any (combination of) numeric spectra and peaks variables.
- Re-organize documentation of filter and subset operations (related to issue
  #288).
@jorainer
Copy link
Member Author

A nice use case for this function would be to filter a LC-MS data set keeping (or removing) all mass peaks that were considered a chromatographic peak by xcms-based preprocessing. This allows e.g. to evaluate the foreground (i.e. all quantified ion signal) and background signal (i.e. background noise, or signal that was not considered a valid chromatographic peak by xcms) of a full data set.

I will later add such a use case to the xcmsTutorials tutorial/workshop.

Copy link
Collaborator

@philouail philouail left a comment

Choose a reason for hiding this comment

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

The codes looks all good to me.

Thanks for reorganizing the documentation I believe it is much more clear now. I put some comments to signals typos and maybe some suggestions. But i think it's great !

#' has to be an available spectra or peaks variable. `<range>` can be a
#' `numeric` of length 2 defining the lower and upper boundary, or a `numeric`
#' two-column matrix (multi-row matrices are also supported, see further
#' below). `filterPeaksRanges(s, mz = c(200, 300))` woudl for example reduce
Copy link
Collaborator

Choose a reason for hiding this comment

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

would*

#' the peaks matrices of the `Spectra` object `s` to mass peaks with an m/z
#' value between 200 and 300. `filterPeaksRanges()` returns the original
#' `Spectra` object with the filter operation added to the processing queue.
#' Thus, the filter gets only applied when the peaks data gets extracted
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe "only" in bold to highlight it

#' or peaks variables) have to match.
#'
#' **Missing value handling**: any comparison which involves a missing value
#' (beingvit a spectra variable value, a peaks variable value or a value
Copy link
Collaborator

Choose a reason for hiding this comment

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

being*

#'
#' **Missing value handling**: any comparison which involves a missing value
#' (beingvit a spectra variable value, a peaks variable value or a value
#' in one of thevprovided ranges) is treated as a logical `FALSE`. For
Copy link
Collaborator

Choose a reason for hiding this comment

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

*the provided

#' @note
#'
#' In contrast to other *filter* functions, this function does not provide a
#' `msLevel.` parameter to apply the filter only on spectra of the specified
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra point at msLevel

#'
#' In contrast to other *filter* functions, this function does not provide a
#' `msLevel.` parameter to apply the filter only on spectra of the specified
#' MS levels. In contrast, to apply no, or different, filters to spectra from
Copy link
Collaborator

Choose a reason for hiding this comment

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

mmm i'm confused by this sentence

R/Spectra.R Outdated
#' operations that reduce the number of spectra in the object as well as filters
#' that reduce the *content* of the `Spectra` object. See section
#' *Filter peaks data* below for functions that filter the peaks data of a
#' `Spectra`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe to make it extremely clear, we can say the number of spectra in the Spectra object will not change with filters until applyProcessing() is run ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess here or at the beginning of the filter section


### Filter spectra data

These functions comprise subset operations that reduce the total number of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add "directly" or immediately" ?

### Filter or aggregate mass peak data

These function filter or aggregate the mass peak data (`peaksData()`) of each
spectrum in a `Spectra` without changing the total number of spectra.
Copy link
Collaborator

Choose a reason for hiding this comment

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

as before I would precise "until the applyProcessing() function in run"

@jorainer
Copy link
Member Author

Thanks @philouail ! I addressed all of your points! Hope documentation is now a bit clearer. Happy for additional/extra feedback, otherwise I proceed and merge tomorrow.

@jorainer jorainer merged commit 681f27d into main Jul 19, 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.

2 participants