Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a new comprehensive vignette demonstrating the analysis of SecA protein hydrogen deuterium exchange mass spectrometry data, along with important bug fixes and code improvements in the hdxstats package.
Changes:
- Added a new case study vignette showing complete HDX-MS analysis workflow for SecA protein data
- Fixed critical bug in
forestPlotfunction where model2 parameters were incorrectly assigned from model1 - Improved
forestPlotfunction to return plot object instead of printing it - Updated package metadata including version bump (1.0 → 1.0.1) and corrected Authors@R format
- Enhanced class flexibility by changing
visslot type from "gg" to "ANY" - Added new imports to NAMESPACE and DESCRIPTION
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/hdxstats-secA-casestudy.Rmd | New comprehensive vignette demonstrating HDX-MS analysis workflow on SecA protein data with functional data analysis approaches |
| R/hdxstat-methods.R | Modified coef method to handle edge case when output has only 1 row by transposing the result |
| R/hdxstat-class.R | Changed vis slot type from "gg" to "ANY" to allow more flexible data storage |
| R/functionalAnalysis-plots.R | Fixed bug using model1 instead of model2, and changed from print to return for better API design |
| NAMESPACE | Added new imports for longFormat, brewer.pal, is, and several packages via import() |
| DESCRIPTION | Updated version to 1.0.1, corrected Authors@R format, expanded description, and added stats to imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(dim(.out)[1] == 1){ | ||
| nm <- colnames(.out)[1] | ||
| .out <- t(.out) | ||
| colnames(.out) <- nm | ||
| } | ||
| rownames(.out) <- c("null", paste0("alt", seq.int(length(object)))) |
There was a problem hiding this comment.
The logic for preserving column names appears incorrect. When transposing a matrix with 1 row, you're saving only the first column name, but after transposing, you should preserve all the original column names as the new row names, not just the first one. Consider saving the entire colnames(.out) vector before transposing, then assigning it as rownames after the transpose.
| if(dim(.out)[1] == 1){ | |
| nm <- colnames(.out)[1] | |
| .out <- t(.out) | |
| colnames(.out) <- nm | |
| } | |
| rownames(.out) <- c("null", paste0("alt", seq.int(length(object)))) | |
| if (dim(.out)[1] == 1) { | |
| ## Preserve all original column names as row names after transposing | |
| nm <- colnames(.out) | |
| .out <- t(.out) | |
| rownames(.out) <- nm | |
| } else { | |
| rownames(.out) <- c("null", paste0("alt", seq.int(length(object)))) | |
| } |
|
|
||
| # Introduction | ||
|
|
||
| Ahdash et al. performed HDX-MS on the baceterial sec translocon, a multi-protein |
There was a problem hiding this comment.
Spelling error: 'baceterial' should be 'bacterial'.
| Ahdash et al. performed HDX-MS on the baceterial sec translocon, a multi-protein | |
| Ahdash et al. performed HDX-MS on the bacterial sec translocon, a multi-protein |
|
|
||
| The data is stored as a .csv, we need to covert it to an object of class | ||
| `QFeatures`. This is performed in a number of steps which we detail below. The | ||
| data is contained within the package and so can be loaded by specificy |
There was a problem hiding this comment.
Spelling error: 'specificy' should be 'specifying'.
| data is contained within the package and so can be loaded by specificy | |
| data is contained within the package and so can be loaded by specifying |
|
|
||
| ``` | ||
|
|
||
| We an use a forest plot to examine the differneces |
There was a problem hiding this comment.
Grammar error: 'We an use' should be 'We can use'.
| We an use a forest plot to examine the differneces | |
| We can use a forest plot to examine the differneces |
| vignette: | | ||
| %\VignetteIndexEntry{SecA hydrogen deuterium exchange mass spectrometry data} | ||
| %\VignetteEngine{knitr::rmarkdown} | ||
| %%\VignetteKeywords{Mass Spectrometry, MS, MSMS, Proteomics, Metabolomics, Infrastructure, Quantitative} %\VignetteEncoding{UTF-8} |
There was a problem hiding this comment.
The vignette metadata has a formatting issue. The VignetteKeywords and VignetteEncoding directives are on the same line. They should be on separate lines for proper parsing. Additionally, there's a double percent sign (%%) before VignetteKeywords which should be a single percent sign (%).
| %%\VignetteKeywords{Mass Spectrometry, MS, MSMS, Proteomics, Metabolomics, Infrastructure, Quantitative} %\VignetteEncoding{UTF-8} | |
| %\VignetteKeywords{Mass Spectrometry, MS, MSMS, Proteomics, Metabolomics, Infrastructure, Quantitative} | |
| %\VignetteEncoding{UTF-8} |
|
|
||
| ```{r, fig.width= 22, fig.height = 15} | ||
| #We need to provide an indication of "difference" so we can examine deprotected | ||
| # or prected regions |
There was a problem hiding this comment.
Spelling error: 'prected' should be 'protected'.
| # or prected regions | |
| # or protected regions |
| comment = c(ORCID = "0000-0001-5669-8506"), | ||
| role = c("aut", "cre"))) | ||
| Description: The hdxstats package enables the analysis and visualisation | ||
| of hydrogen deuterium exchange mass spectromery experiments. It supports |
There was a problem hiding this comment.
Spelling error: 'spectromery' should be 'spectrometry'.
| of hydrogen deuterium exchange mass spectromery experiments. It supports | |
| of hydrogen deuterium exchange mass spectrometry experiments. It supports |
| toc_float: yes | ||
| abstract: "This vignette describes how to analyse a mass-spectrometry based hydrogen | ||
| deuterium exchange experiment, in particular we focus on empirical Bayes functional | ||
| models and visualisations. This vignette descibes a case-study for analysing |
There was a problem hiding this comment.
Spelling error: 'descibes' should be 'describes'.
| models and visualisations. This vignette descibes a case-study for analysing | |
| models and visualisations. This vignette describes a case-study for analysing |
| membrane. Here we analyse this experiment using our functional data analysis | ||
| approaches | ||
|
|
||
| # loading packges |
There was a problem hiding this comment.
Spelling error: 'packges' should be 'packages'.
| # loading packges | |
| # loading packages |
|
|
||
| ``` | ||
|
|
||
| We an use a forest plot to examine the differneces |
There was a problem hiding this comment.
Spelling error: 'differneces' should be 'differences'.
| We an use a forest plot to examine the differneces | |
| We an use a forest plot to examine the differences |
No description provided.