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/mztabm #502

Merged
merged 4 commits into from
Apr 14, 2020
Merged

Feature/mztabm #502

merged 4 commits into from
Apr 14, 2020

Conversation

sneumann
Copy link
Contributor

This PR adds

  1. a possibly breaking change where the special characters like space, [ and ] are not modified upon readin, see discussion in Valid row/colnames after reading mzTab #501
  2. A write.mzTab method which writes an MzTab object back to disc.

Points for discussion:

  1. Is there any objection to the check.names=FALSE change ?
  2. is the implementation of write.mzTab() as method OK ?
  3. Which "flavour" of mzTab do we want to write w.r.t. to number of columns in the different sections ? Should it be rectangular ? I think there are cases where we need to "trim" empty columns before creating the MzTab slots.

Yours, Steffen

@lgatto
Copy link
Owner

lgatto commented Apr 13, 2020

Could you please send a PR per requested change.

  • The first one would be the change to check.names = FALSE, which I am happy with. With that PR in, it will be possible to check that there isn't anything that breaks.
  • As for write.MzTab, it should be writeMzTab and a simple function will do.
  • We try to adhere to coding style, which I am happy to help fix. But that will only be easy if you provide specific PRs (see first point above).

@codecov-io
Copy link

Codecov Report

Merging #502 into master will increase coverage by 0.14%.
The diff coverage is 92.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
+ Coverage   71.80%   71.94%   +0.14%     
==========================================
  Files          82       82              
  Lines        8225     8274      +49     
==========================================
+ Hits         5906     5953      +47     
- Misses       2319     2321       +2     
Impacted Files Coverage Δ
R/readWriteMzTab.R 80.00% <88.23%> (+1.76%) ⬆️
R/MzTab.R 90.51% <100.00%> (+3.51%) ⬆️

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 8e5b962...af35464. Read the comment docs.

@@ -11,7 +11,7 @@ test_that("MzTab creation and accessors", {
expect_null(show(xx))
## Accessors
expect_is(metadata(xx), "list")
expect_identical(length(metadata(xx)), 64L)
expect_identical(length(metadata(xx)), 65L)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that the previous implementation without headers=FALSE swallowed the first line of the MTD section. After fixing that there is one more metadata line.

@lgatto lgatto merged commit 0c0d66d into lgatto:master Apr 14, 2020
@lgatto
Copy link
Owner

lgatto commented Apr 14, 2020

Are there more PRs to come, or should I push to Bioconductor now?

@sneumann
Copy link
Contributor Author

There are no imminent PRs. What's left is a decision about the dimensionality of things.
Some mzTab files have columns of empty tabs to the right of the actual data.frame.
R happily imagines some names like "V8" "V9" for such columns. This needs to be discussed
with mzTab folks, possibly me reading specs again. I would opt for some (optional ?)
trimming of the MzTab@SmallMolecules data.frame upon reading.
But as said, that's not coming soon. Yours, Steffen

@lgatto
Copy link
Owner

lgatto commented Apr 14, 2020

Pushed to Bioc.

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.

None yet

3 participants