[JOSS REVIEW]: subMALDI - tystan

[tystan]

Just trying to create a checklist like @Jspaezp did. Please bear with me, I will delete this if it doesn't go to plan!

Issue #2694

---

Review checklist for @tystan

Conflict of interest

• [x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

• [x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• [x] Repository: Is the source code for this software available at the repository url?
  • yes, here
• [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • yes, GNU GENERAL PUBLIC LICENSE Version 3
• [x] Contribution and authorship: Has the submitting author (@wesleyburr) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • yes, @wesleyburr, @castels and Kristen Yeh have made most of the commits
• [x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
  • yes, "JOSS’ minimum allowable contribution should represent not less than three months of work for an individual" would seem to have been met, plus some

Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?
  • Yes, no issues
• [x] Functionality: Have the functional claims of the software been confirmed?
  • sample workflow online, sample workflow vignette, Processing with subMALDI online and Processing with subMALDI vignette now checked - all OK for version 1.0.1.
  • I would suggest changing the default argument min_mz = 0 in plotSpectra() to be changed to min_mz = min(dat[spectra_cols]) to avoid the warning message, pasted below, whenever plotting is performed on spectra with m/z values greater than 0. (or alternatively, remove the warning message for min_mz = 0. Either way, it is not ideal getting warnings for default usage) Warning message: In plotSpectra(norm_spec, mass_dat = "full_mz", spectra_cols = c("Before1", : Specified value of 'min_mz' is beyond the mass range in 'dat'. Defaulting to the minimum mass.
  • line 377 of the sample workflow produces the warning 2: Removed 116926 row(s) containing missing values (geom_path). from ggplot2. Either no warning should be produced (trim the dataset before plotting?) or produce a more helpful warning from subMALDI
  • testthat unit tests exist for confirming the functional claims
• [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Yes well presented in the README.md landing page
• [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Yes, clear instructions. The only thing I would add is a hyperlink to the Releases page (https://github.com/wesleyburr/subMaldi/releases) to the sentence "To install this package in R, begin by downloading the source copy of the package from the releases folder"
• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Yes, the 2 vignettes cover that off nicely.
• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • There are help (.Rd) files for the functions in the man/ directory
  • they contain argument, output and example code - looks great
• [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Yes units tests in the testthat/ directory. However the files test-plotSpectra.R and test-readcsvDir.R are empty. If they are methods that don't need/can't be unit tested, commented out notes explaining why seems sensible. Additionally, test-baselineCorr.R seems incomplete but I think the unit testing has been done in the other test_base_*.R tests.
  • However, running R CMD check subMALDI_1.0-1.tar.gz on the package produced warnings/errors such as: (1) Packages in Depends field not imported from: 'RColorBrewer' 'reshape2' These packages need to be imported from (in the NAMESPACE file for when this namespace is loaded but not attached. (2) Undocumented code objects: 'norm_RMS' 'norm_TIC' 'norm_custimp' 'norm_custom' 'norm_max' 'norm_max_set' 'norm_median' 'norm_quantile' 'norm_rel_TIC' 'norm_stdev' (3) * checking for code/documentation mismatches ... WARNING Functions or methods with usage in documentation object '.normMethod_RMS' but not in code: '.normMethod_RMS' (4) * checking examples ... ERROR Running examples in 'subMALDI-Ex.R' failed[...] + cleanEx() Error: unexpected symbol in:"cleanEx" Execution halted etc
• [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support
  • 1) yes, link in last line of README.md 2) I assume github issues is enough 3) The help files contain the authors and emails

Software paper

• [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • Yes explains what mass spec data are. There are proprietary software packages but this is open source, has functionality for the vast array of pre-processing steps and allows interrogation of the spectra at the (non-expert) user level
• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • similar to above. The other R packages in MSnbase and MALDIquant are very different, and subMALDI definitely provides a simpler interface that non-R users would appreciate
• [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Yes a good list of other proprietary (Bruker Daltonics) and open source packages in various languages (Python, R, Matlab, etc)
• [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • No issues, well written
• [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
  • References look great, only exception is the Gibb, S (2016) reference purely because the URL goes off the side of the page but that's a formatting thing not a content thing.

[wesleyburr]

Functionality fixed.

[wesleyburr]

Found all kinds of things that had slipped through - hadn't been religious about R CMD check as the refactor was happening. Thanks for flagging this - found some functionality issues and documentation issues that would have been missed otherwise. R CMD check passes with no warnings or errors now.

I think the fixes for you are done!

[tystan]

Hi @wesleyburr . Thanks for this. Have replied on the main review comment chain: https://github.com/openjournals/joss-reviews/issues/2694#issuecomment-916103675