fix mzIdentML import for MGF based identification files; see issue #42

[sgibb]

This PR is WIP. @lgatto's suggestion #42 (comment) and the documentation are still missing. I will leave a comment if this PR is finished.

[sgibb]

This PR may breaks the API because the generic for addIdentificationData was changed:

# before PR
setGeneric("addIdentificationData", function(object, ...) standardGeneric("addIdentificationData"))
# PR
setGeneric("addIdentificationData", function(object, id, ...) standardGeneric("addIdentificationData"))

Additionally the name of the second argument was filenames before (is id now).

idSummary is currently not working (because mzIDCollection contains no hint about the corresponding id files; see thomasp85/mzID#27), some tests fail (all regarding the identification file problem) and the documentation is still missing.

[sgibb]

@lgatto a few questions:

1. Do we really want to add the filenames of the identification files to MSnExp@MSnProcess@files slot (via fileNames(msexp) <- basename(mzID::files(id)$raw$location))? This would not work for the general data.frame if it contains no "idFile" column (that's why I removed it from the current code in this PR, but it could be added easily).
2. By adding the new overloaded methods addIdentificationData we break the backward compatibility. How we want to deal with this fact (the second argument is now id instead of filenames)?

[sgibb]

A 3. questions same problem as 2.: backward compatibility:
The merge columns are now fcol = c("spectrum.file", "acquisition.number"), icol = c("spectrumFile", "acquisitionnum") (was fcol = c("file", "acquisition.number"), icol = c("spectrumFile", "acquisitionnum") before but it wasn't visible to the user).

Both fcol columns are generated from the MSnExp as follows:

fd$spectrum.file <- basename(fileNames(object)[fromFile(object)])
fd$acquisition.number <- acquisitionNum(object)

The icol columns are present in the mzID output. IMHO it is easier for the user to create his own spectrumFile and acquisitionNum columns (if he want to create his own data.frame and don't use mzID) than creating a file column (file id; position of the file in the MSnExp@MSnProcess@files slot). The latter could be done automatically for mzID but the user has to do it himself for his own data.frames.

This change breaks the backward compatibility. Should not be a real problem because nobody could add his own data.frame yet. (But if somebody works with the fData(msexp)$identFile column he will have trouble.)

The output of idSummary would change as well. Instead of file id numbers there would be filenames.

What do you think?

[lgatto]

Do we really want to add the filenames of the identification files to MSnExp@MSnProcess@files slot (via fileNames(msexp) <- basename(mzID::files(id)$raw$location))? This would not work for the general data.frame if it contains no "idFile" column (that's why I removed it from the current code in this PR, but it could be added easily).

Would it be possible to just ignore the identification filename if there is no idFile column? We don't want this to become overly complicated, so I would also consider dropping it altogether.

[lgatto]

By adding the new overloaded methods addIdentificationData we break the backward compatibility. How we want to deal with this fact (the second argument is now id instead of filenames)?

Yes, but I think we gain much from this. There have been 2 or 3 issues about adding id data, so some flexibility is welcome. In the future, I would also consider add a method for identification from mzR.

[lgatto]

A 3. questions same problem as 2.: backward compatibility:

Same as for 2 - go ahead, break it ;-)

[sgibb]

IMHO this PR is ready to merge if mzID 1.5.2 is on bioc. Therefore @thomasp85 has to export his mzID and mzIDCollection classes or accept this PR: thomasp85/mzID#28

The identification filenames are ignored now (was issue 1.). The backward compatibility is broken (were 2. and 3.).

#42 is solved with this PR by the following clumsy call:

library("MSnbase")

msexp <- readMgfData("3_2.mgf")
msexp <- addIdentificationData(msexp, "3_2.msgf.mzid",
                               fcol=c("spectrum.file", "TITLE"),
                               icol=c("spectrumFile", "spectrum title"))

Theoretical #43 should be solved as well but I don't know the correct columns.

[thomasp85]

Will do tomorrow - lot on my plate atm.

Den 28/01/2015 kl. 22.56 skrev Sebastian Gibb notifications@github.com:

IMHO this PR is ready to merge if mzID 1.5.2 is on bioc. Therefore @thomasp85 has to export his mzID and mzIDCollection classes or accept this PR: thomasp85/mzID#28

The identification filenames are ignored now (was issue 1.). The backward compatibility is broken (were 2. and 3.).

#42 is solved with this PR by the following clumsy call:

library("MSnbase")

msexp <- readMgfData("3_2.mgf")
msexp <- addIdentificationData(msexp, "3_2.msgf.mzid",
fcol=c("spectrum.file", "TITLE"),
icol=c("spectrumFile", "spectrum title"))
Theoretical #43 should be solved as well but I don't know the correct columns.

—
Reply to this email directly or view it on GitHub.

[lgatto]

Sorry, dump question here

msexp <- addIdentificationData(msexp, "3_2.msgf.mzid",
                               fcol=c("spectrum.file", "TITLE"),
                               icol=c("spectrumFile", "spectrum title"))

but why do you have spectrum.file/spectrumFile and TITLE/spectrum title? Is the matching not done on one only?

[sgibb]

In this particular case spectrum.file/spectrumFile is indeed superfluous. The following would be enough:

msexp <- addIdentificationData(msexp, "3_2.msgf.mzid",
                               fcol=c("TITLE"),
                               icol=c("spectrum title"))

I had multiple file support in mind. But I have seen that readMgfData doesn't support multiple files. That's why it should be safe to use just TITLE/spectrum title.

mzID 1.5.2 is on bioc; If you agree this PR could be merged.

[lgatto]

If you agree this PR could be merged.

Sure, go ahead!

[lgatto]

FYI: I have now pushed the latest version to the Bioc svn server.