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

bin_Spectra bug #190

Closed
mtrnka opened this issue Feb 8, 2017 · 3 comments
Closed

bin_Spectra bug #190

mtrnka opened this issue Feb 8, 2017 · 3 comments
Assignees
Labels

Comments

@mtrnka
Copy link

mtrnka commented Feb 8, 2017

Hi all,
First of all, thanks for this project. I'm working on some spectral library matching and have found all the RforProteomics libraries very useful.

I found a bug in functions-Spectrum.R. The function bin_Spectra calculates the breaks from the combined range of the two spectra objects. It then passes the breaks to function bin_Spectrum. bin_Spectrum contains an if clause that checks that the mz range of the spectrum actually falls within breaks and if not extends the breaks. The way it is coded it applies this only to the spectrum that exceeds the range leaving the other spectrum with the original range.

This is causing problems for me as I am trying to extract out peaks based on dot-product matching to reference spectra against an entire mzML file and inevitably cases crop up where it is trying to calculate dotproducts on nonconformable matrices because of the difference in bin-sizes.

I've fixed this issue for myself as noted below, but wanted to give you guys a heads up.

bin_Spectra <- function(object1, object2, binSize = 1L,
                        breaks = seq(floor(min(c(mz(object1), mz(object2)))),
                                     ceiling(max(c(mz(object1), mz(object2))))+binSize,
                                     by = binSize)) {
    return(list(bin_Spectrum(object1, binSize = binSize, breaks = breaks),
                bin_Spectrum(object2, binSize = binSize, breaks = breaks)))
}
@sgibb sgibb self-assigned this Feb 8, 2017
@sgibb sgibb added the bug label Feb 8, 2017
@sgibb
Copy link
Collaborator

sgibb commented Feb 9, 2017

Thanks for reporting this! The bug was introduced while fixing #137. Here is an MWE:

s1 <- new("Spectrum2", mz=1:4, intensity=1:4)
s2 <- new("Spectrum2", mz=1:5, intensity=1:5)

lapply(MSnbase:::bin_Spectra(s1, s2), 
       function(x)rbind(mz=mz(x), intensity=intensity(x)))
# [[1]]
#           [,1] [,2] [,3] [,4]
# mz         1.5  2.5  3.5  4.5
# intensity  1.0  2.0  3.0  4.0
# 
# [[2]]
#           [,1] [,2] [,3] [,4] [,5]
# mz         1.5  2.5  3.5  4.5  5.5
# intensity  1.0  2.0  3.0  4.0  5.0

expected output:

# [[1]]
#           [,1] [,2] [,3] [,4] [,5]
# mz         1.5  2.5  3.5  4.5  5.5
# intensity  1.0  2.0  3.0  4.0  0.0
#
# [[2]]
#           [,1] [,2] [,3] [,4] [,5]
# mz         1.5  2.5  3.5  4.5  5.5
# intensity  1.0  2.0  3.0  4.0  5.0

@sgibb
Copy link
Collaborator

sgibb commented Feb 10, 2017

"Fixing" a bug (#137) by introducing two new bugs (#190, #191). Not too bad 😞 Hopefully I didn't introduced four new ones by fixing these two.

@lgatto
Copy link
Owner

lgatto commented Feb 10, 2017

Thanks for your work, @sgibb!

lgatto pushed a commit that referenced this issue Apr 9, 2017
* master:
  don't call nFeatures in combineFeatures
  document nFeatures when combining
  new nFeatures function - close issue 192
  document aggvar in news
  add aggvar man
  add hint about changing breaks on the RHS because of #190
  addapt unit test for bin_Spectrum (see #191)
  remove useless :::
  add NEWS entry for fixed binning
  fix breaks calculation for binning single (closes #191) and multiple (closes #190) spectra
  add aggvar test
  aggvar, ragnars' offspring function will rip your guts and serve them to the gods
  add agg function - currently unexported
  coding style
  new github devel version

From: Laurent <lg390@cam.ac.uk>

git-svn-id: https://hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks/MSnbase@126926 bc3139a8-67e5-0310-9ffc-ced21a209358
lgatto pushed a commit that referenced this issue Sep 7, 2017
* master:
  don't call nFeatures in combineFeatures
  document nFeatures when combining
  new nFeatures function - close issue 192
  document aggvar in news
  add aggvar man
  add hint about changing breaks on the RHS because of #190
  addapt unit test for bin_Spectrum (see #191)
  remove useless :::
  add NEWS entry for fixed binning
  fix breaks calculation for binning single (closes #191) and multiple (closes #190) spectra
  add aggvar test
  aggvar, ragnars' offspring function will rip your guts and serve them to the gods
  add agg function - currently unexported
  coding style
  new github devel version

From: Laurent <lg390@cam.ac.uk>

git-svn-id: file:///home/git/hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks/MSnbase@126926 bc3139a8-67e5-0310-9ffc-ced21a209358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants