sneumann / mzR

This is the git repository matching the Bioconductor package mzR: parser for netCDF, mzXML, mzData and mzML files (mass spectrometry data)
40 stars 26 forks source link

mis-documentation and removal of mz5 support in 2014-07-11 / 84a4b1664 #118

Closed HinTak closed 6 years ago

HinTak commented 6 years ago

Some of the online documentation of mzR (version 2.6.2?) claims that mzR support mz5 . I am using the development version of R and therefore the development version of bioconductor. Found that it is not working:

> sessionInfo()
R Under development (unstable) (2017-08-10 r73083)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Fedora 26 (Workstation Edition)
...
...
> msA0 <- openMSfile("group_A/sample_A0.mz5", backend="pwiz")
Error in pwizModule$open(filename) :
  [ReaderFail] [Reader_mz5::read] library was not built with mz5 support.
>

Found that you explicitlly disable MZ5 support in src/Makevars:

src/Makevars:128:PWIZ_CPPFLAGS=-I./boost_aux/ -I./boost/ -I. -D_LARGEFILE_SOURCE -DHAVE_PWIZ_MZML_LIB -D_NODEBUG -DWITHOUT_MZ5  

So I traced the origin of that line backwards, and apparently it was done over two years ago as part of GSoC:

commit 84a4b1664f27a82bf12957dadc86443dd7c38705
Author: s.neumann <s.neumann@bc3139a8-67e5-0310-9ffc-ced21a209358>
Date:   Sun Sep 28 18:06:01 2014 +0000

    Commit made by the Bioconductor Git-SVN bridge.
    Consists of 130 commits.

    Commit information:

    Commit id: 289dc240065888efcaa22ae75951e81f94fc800f

        Fix package description after merge

    Committed by: Steffen Neumann
    Author Name: Steffen Neumann
    Commit date: 2014-09-28 17:21:35 +0200
    Author date: 2014-09-28 17:21:35 +0200

    Commit id: 88248e8d6d86bb54dc260f0c55608dd1a949ade4

        merged gsoc into master

...
    Committed by: qkou
    Author Name: qkou
    Commit date: 2014-07-11 01:48:50 -0400
    Author date: 2014-07-11 01:48:50 -0400

    Commit id: 5f49d13b736cebd65484a5993cdcce253853d4e7

        remove mz5

Why is that?

In any case, I think if you remove a past useful feature you probably should mention it in the top level README.

HinTak commented 6 years ago

FYI, dev version of Bioconductor ships mzR_2.10.0 .

HinTak commented 6 years ago

This seems to be some sort of confusion - mz5 support was removed in May 26 18:28:05 2014, added back in Jun 25 22:13:35 2014, and removed again in July 11 2014.

HinTak commented 6 years ago

I am not sure why I was removed - the only guess I can make is that it requires the cpp version of hdf5, which is a rare library to install. Anway, I seem to have come up with about half-a-dozen lines of code change which works:

> library(mzR)
Loading required package: Rcpp
> msA0 <- openMSfile("group_A/sample_A0.mz5", backend="pwiz")
> msA0
Mass Spectrometry file handle.
Filename:  sample_A0.mz5 
Number of scans:  5416 
> summary(msA0)
 Length   Class    Mode 
   5416 mzRpwiz      S4 

> colnames(header(msA0))
 [1] "seqNum"                   "acquisitionNum"          
 [3] "msLevel"                  "polarity"                
 [5] "peaksCount"               "totIonCurrent"           
 [7] "retentionTime"            "basePeakMZ"              
 [9] "basePeakIntensity"        "collisionEnergy"         
[11] "ionisationEnergy"         "lowMZ"                   
[13] "highMZ"                   "precursorScanNum"        
[15] "precursorMZ"              "precursorCharge"         
[17] "precursorIntensity"       "mergedScan"              
[19] "mergedResultScanNum"      "mergedResultStartScanNum"
[21] "mergedResultEndScanNum"   "injectionTime"           
> 

So I write a proper log entry and issue a pull.

HinTak commented 6 years ago

The travis-ci server does not have hdf5_cpp headers. One way of improving it is to make the configure script auto-detect.

HinTak commented 6 years ago

Why is the issue closed? There is at least the issue of properly documenting the lack of mz5 support, even if the current state is considered "acceptable".