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

Feature/update pwiz 3 0 21263 #253

Closed sneumann closed 2 years ago

sneumann commented 2 years ago

Update to Proteowizard 3_0_21263 Removed RAMP backend, dropping ability to read mzData

Closes #250, Revisit #247, #245, #235, #233, and maybe others.

jorainer commented 2 years ago

Just checked - this PR does not contain the changes/cleanup I made in the C++ code reading (header) data using proteowizard (#252). But I can later adapt that PR - let's only have the proteowizard update in this one.

jorainer commented 2 years ago

After having another though I think it makes sense to change the current behaviour of the header call and always return a data.frame. Also, since the CDF backend seems to always return a data.frame and also the (obsolete) Ramp backend returned always a data.frame - so mayby I added that feature at some point accidentally (?).

I would thus suggest to remove the lines:

              if (length(scans) == 1) {
                  ## Convert data.frame to list to be conform with old code
                  res <- as.list(res)
              }

in the setMethod("header", c("mzRpwiz", "numeric"), method in the methods-mzRpwiz.R file. Do you agree @lgatto and @sneumann ? Could you eventually do that in this PR @sneumann ?

lgatto commented 2 years ago

Yes, I think the header should always be a data.frame.

jorainer commented 2 years ago

I'll make a PR to this branch, so no need to manually change anything @sneumann .

jorainer commented 2 years ago

PR #254 (into this branch) adds the above described changes (i.e. header always returning a data.frame).

jorainer commented 2 years ago

Hi @sneumann , with the recent changes I can install, but I get the following WARNINGS (which cause the github action to fail):

* checking for portable file names ... WARNING
Found the following file with a non-portable file name:
  src/boost/serialization/collection_size_type copy.hpp
These are not fully portable file names.
See section ‘Package structure’ in the ‘Writing R Extensions’ manual.
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘mzR’ can be installed ... WARNING
Found the following significant warnings:
  pwiz/data/proteome/Modification.cpp:89:5: warning: 'auto_ptr<pwiz::chemistry::Formula>' is deprecated [-Wdeprecated-declarations]
  pwiz/utility/misc/TabReader.cpp:44:22: warning: assigning field to itself [-Wself-assign-field]
See ‘/Users/jo/Projects/git/jorainer/mzR.Rcheck/00install.out’ for details.

don't know if there is something that could be done for these.

sneumann commented 2 years ago

Hi, while the Warnings are a nuisance, I'd like to address the errors on all arches first. On Windows I get https://github.com/sneumann/mzR/runs/4342043079?check_suite_focus=true#step:20:2932 which sounds like quite some compiler complaints. Maybe @hechth might have an idea ? Yours, Steffen

hechth commented 2 years ago

@sneumann Thanks for letting me know - from the log I can see that some macros are undefined or mapped to the wrong type, but this is not a very helpful. I' currently quite busy, so I can't promise when I will find the time to have a look at this.

sneumann commented 2 years ago

Hi, @uly55e5 offered to have a look, thanks David ! Yours, Steffen

sneumann commented 2 years ago

Hi, we got things to work on Windows (thanks @uly55e5 !) on the errors on macOS are a failure to set-up R-devel in the first place. It did work some time ago on macOS, we can hope it still does. SInce mzR on BioC devel is broken anyway, I'll merge now and we pick up the pieces from there. Yours, Steffen