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

Some updates and bug fixes #761

Merged
merged 7 commits into from
Aug 9, 2024
Merged

Some updates and bug fixes #761

merged 7 commits into from
Aug 9, 2024

Conversation

jorainer
Copy link
Collaborator

@jorainer jorainer commented Aug 6, 2024

This PR adds:

  • fix for issue Chromatograms of MS1 data with two different scan events per file #755 (chromatogram() call with msLevel = 2L); improve documentation (and vignette) for this.
  • change the estimatePrecursorIntensity() function to a method and import the generic from ProtGenerics. This ensures we can have different implementations for XcmsExperiment, XCMSnExp and also avoid clashes with the implementation in Spectra for Spectra objects.
  • add a function that allows coercion of a XcmsExperiment to XCMSnExp for backward compatibility.
  • fix GHA-unit tests

- Support coercing from `XcmsExperiment` to `XCMSnExp` using
  `setAs(object, "XCMSnExp")` (issue #752).
- Change `estimatePrecursorIntensity()` to a method and add an implementation
  for `MsExperiment`.
- `chromatogram` with `msLevel = 2L` does by default extract chromatographic
  data of all MS2 spectra, independently of whether different m/z isolation
  windows were used. For data with isolation windows, the respective isolation
  window can be set/defined with the `isolationWindowTargetMz` parameter.
- Update documentation to describe this behaviour better.
@jorainer jorainer requested a review from sneumann August 6, 2024 07:51
brew install imagemagick@6

## For textshaping, required by ragg, and required by pkgdown
brew install harfbuzz fribidi
Copy link
Owner

Choose a reason for hiding this comment

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

brew install harfbuzz fribidi seriously ? Somebody was creative there :-) Certainly very google'able...

DESCRIPTION Outdated
@@ -1,5 +1,5 @@
Package: xcms
Version: 4.3.2
Version: 4.3.4
Copy link
Owner

Choose a reason for hiding this comment

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

Are we skipping 4.3.3 ? I couldn't find a 4.3.3 in a PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, my mistake. Somehow I forgot to make the 4.3.3 PR. I will revert to 4.3.3 for this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed now.

@@ -54,7 +54,7 @@ Imports:
methods,
Biobase,
BiocGenerics,
ProtGenerics (>= 1.35.4),
ProtGenerics (>= 1.37.1),
Copy link
Owner

@sneumann sneumann Aug 9, 2024

Choose a reason for hiding this comment

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

That's needed because only the newer has a generic for estimatePrecursorIntensity ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly. The estimatePrecursorIntensity generic was added to ProtGenerics version 1.37.1

@@ -63,7 +63,7 @@ Imports:
MsCoreUtils (>= 1.15.5),
MsFeatures,
MsExperiment (>= 1.5.4),
Spectra (>= 1.13.7),
Spectra (>= 1.15.7),
Copy link
Owner

Choose a reason for hiding this comment

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

I Guess needed because only the newer has estimatePrecursorIntensity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, because for MsExperiment/XcmsExperiment we're using the estimatePrecursorIntensity,Spectra method, while for the older XCMSnExp object we use a different one.

Copy link
Owner

@sneumann sneumann left a comment

Choose a reason for hiding this comment

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

Looks good to me, and I learned about @noRd today ;-)

@sneumann sneumann merged commit 08e1649 into devel Aug 9, 2024
1 of 3 checks passed
@jorainer jorainer deleted the issue_755 branch August 9, 2024 11:17
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