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

Feature/update pwiz 3 0 21263 #253

Merged
merged 41 commits into from
Jan 25, 2022
Merged

Feature/update pwiz 3 0 21263 #253

merged 41 commits into from
Jan 25, 2022

Conversation

sneumann
Copy link
Owner

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

lgatto commented Nov 25, 2021

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

@jorainer
Copy link
Collaborator

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

- Fix header for pwiz backend to always return a `data.frame`.
- Update documentation.
@jorainer
Copy link
Collaborator

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

@jorainer
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link

hechth commented Dec 8, 2021

@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
Copy link
Owner Author

sneumann commented Dec 8, 2021

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

@sneumann
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update pwiz code base
5 participants