search

lgatto / MSnbase

Base Classes and Functions for Mass Spectrometry and Proteomics
http://lgatto.github.io/MSnbase/
124 stars 50 forks source link

Rename Spectra and Chromatograms #516

Closed jorainer closed 4 years ago

jorainer commented 4 years ago
jorainer commented 4 years ago

I don't quite understand why travis fails - all running smoothly locally.

codecov-commenter commented 4 years ago

Codecov Report

Merging #516 into master will not change coverage. The diff coverage is 92.59%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #516   +/-   ##
=======================================
  Coverage   72.35%   72.35%           
=======================================
  Files          81       81           
  Lines        8238     8238           
=======================================
  Hits         5961     5961           
  Misses       2277     2277
Impacted Files Coverage Δ
R/DataClasses.R 75.00% <ø> (ø)
R/methods-Chromatogram.R 78.26% <ø> (ø)
R/functions-Spectrum.R 88.57% <50.00%> (ø)
R/methods-MChromatograms.R 77.30% <90.00%> (ø)
R/functions-MChromatograms.R 97.11% <100.00%> (ø)
R/functions-MSpectra.R 100.00% <100.00%> (ø)
R/methods-MSnExp.R 75.40% <100.00%> (ø)
R/methods-MSpectra.R 95.28% <100.00%> (ø)
R/readChromData.R 91.95% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 394f29e...00d3ca1. Read the comment docs.

jorainer commented 4 years ago

OK, warnings fixed.

lgatto commented 4 years ago

What about also renaming Chromatogram to MChromatogram? I know it isn't a matrix, but it would make the relation Chromatogram -> MChromatograms more explicit and it would also avoid ambiguity with MSnbase::Chromatogram and Spectra::Chromatograms.

jorainer commented 4 years ago

Hm, I don't think that would be necessary. To be consistent we should then also rename Spectrum, Spectrum1, Spectrum2 to MSpectrum, ...

lgatto commented 4 years ago

There is no Spectrum* in Spectra (and there will never be).

jorainer commented 4 years ago

But that's the same with Chromatogram - we will not have a Chromatograms::Chromatogram class. In general I'm OK with renaming MSnbase::Chromatogram to MSnbase::MChromatogram, I just wanted to do as little changes as possible, because these class renamings might cause downstream problems in other packages.

lgatto commented 4 years ago

It isn't critical at this stage, I just want to avoid having to do another such refactoring another time.

Regarding the Spectrum class, do you remember the discussion that lead to this. If this ever materialises, we might very well hit a clash with MSnbase. I thought we would stay on the safe side with anticipating anything, even unlikely, by renaming to MChromatogram.

lgatto commented 4 years ago

I looked at the changes and am doing some local checks for the current PR. Will merge soon.