proFIA

[adelabriere]

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

• Repository: https://github.com/adelabriere/proFIA

Confirm the following by editing each check box to '[x]'

• [x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
• [x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
• [x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
• [x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.

I am familiar with the essential aspects of Bioconductor software management, including:

• [x] The 'devel' branch for new packages and features.
• [x] The stable 'release' branch, made available every six months, for bug fixes.
• [x] Bioconductor version control using Subversion (optionally via GitHub).

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

[bioc-issue-bot]

Hi @adelabriere

Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.

The DESCRIPTION file for this package is:

Package: proFIA
Type: Package
Title: Preprocessing of FIA-HRMS data
Version: 0.99.0
Date: 2016-05-04
Author: Alexis Delabriere and Etienne Thevenot.
Maintainer: Alexis Delabriere <alexis.delabriere@cea.fr>
biocViews: MassSpectrometry, Metabolomics, Lipidomics, Preprocessing,
    PeakDetection, Proteomics
Depends:
    R (>= 3.3.0)
Imports:
    stats,
    graphics,
    utils,
    grDevices,
    methods,
    xcms,
    pracma,
    Biobase,
    FNN,
    minpack.lm,
    BiocParallel
Suggests:
    ropls,
    BiocGenerics,
    plasFIA,
    knitr,
VignetteBuilder: knitr
Description: Flow Injection Analysis coupled to High-Resolution Mass
    Spectrometry is a promising approach for high-throughput metabolomics. FIA-
    HRMS data, however, cannot be pre-processed with current software tools which
    rely on liquid chromatography separation, or handle low resolution data only.
    Here we present the proFIA package, which implements a new methodology to
    pre-process FIA-HRMS raw data (netCDF, mzData, mzXML, and mzML)  including noise 
    modelling and injection peak reconstruction, and generate the peak table. The workflow
    includes noise modelling, band detection and filtering then signal matching and missing
    value imputation. The peak table can then be exported as a .tsv file for further analysis.
    Visualisations to assess the quality of the data and of the signal made are easely
    produced.
License: CeCILL
NeedsCompilation: yes
Packaged: 2016-05-24 19:11:28 UTC;
RoxygenNote: 5.0.1
Collate:
    'Denoising.R'
    'cWrapper.R'
    'noiseEstimator.R'
    'classContainer.R'
    'fastMatchPpm.R'
    'findPeaksFIA.R'
    'proFIA-package.R'

[bioc-issue-bot]

Your package has been approved for building. Your package is now submitted to our queue.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160919144206.html

[hpages]

Hi Alexis,

Despite bioc-issue-bot reporting that "the package built without errors or warnings on all platforms", there was an issue with our automated single package builder that prevented it from performing the usual build/check of the proFIA package. You can see this by following the link to the build report: the report is actually empty. I suspect this is due to the fact that the single package builder was not able to install suggested package plasFIA. Can you please submit plasFIA in this same issue? Thanks, H.

[adelabriere]

AdditionalPackage: https://github.com/adelabriere/plasFIA

[bioc-issue-bot]

Dear @adelabriere ,

I could not find a DESCRIPTION file in the default branch of the GitHub repository at https://github.com/adelabriere/plasFIA . This repository should contain an R package.

[adelabriere]

AdditionalPackage: https://github.com/adelabriere/plasFIA

[bioc-issue-bot]

Hi @adelabriere,

Starting build on additional package https://github.com/adelabriere/plasFIA.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your additional package repository will NOT trigger a new build.

The DESCRIPTION file of this additional package is:

Package: plasFIA
Version: 0.99.0
Date: 2016-08-30
Title: FIA-HRMS plasma dataset
Author: Alexis Delabriere, Etienne Thevenot, Ulli Hohenester and Christophe Junot.
Maintainer: Alexis Delabriere <alexis.delabriere@cea.fr>
Depends: proFIA
ZipData: no
Description: Positive Ionization FIA-HRMS data of human plasma
spiked with a pool of 40 compounds acquired in FIA-HRMS mode
on an orbitrap fusion. plasFIA also include the result of the
processing by the proFIA package with adapted parameters for
    an Orbitrap Fusion.
biocViews: ExperimentData, MassSpectrometryData
License: LGPL
Packaged: 2015-10-15 16:36:38 UTC;
NeedsCompilation: no
RoxygenNote: 5.0.1

[adelabriere]

Thanks,

This is done I hope that it is going to work this time.

Regards Alexis

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/plasFIA_buildreport_20160920034145.html

[adelabriere]

Well visibly the report is once again empty, the issue that I have is that proFIA require plasFIA to build the vignette and to run examples and unitary tests plus there is an S4 object in the data of plasFIA which forces me to put proFIA in depends of plasFIA. and proFIA needs plasFIA to build vignette.

I asked the question on the bioc-devel list but at the moment no solution cames out.

[hpages]

Yes, the solution was to put plasFIA in the Suggests field, as you did. The fact that the automated single package builder produces an empty report is an issue with the single package builder itself, not with your packages. I reported the issue to the single package builder maintainers.

In the mean time, I built and checked proFIA on my Linux machine (64-bit Ubuntu 14.04) and got the following:

hpages@latitude:~$ Rbiocdev CMD check proFIA_0.99.0.tar.gz
* using log directory ‘/home/hpages/pkgreviews/proFIA.review/github/proFIA.Rcheck’
* using R version 3.3.0 Patched (2016-05-12 r70604)
* using platform: x86_64-pc-linux-gnu (64-bit)
* using session charset: UTF-8
* checking for file ‘proFIA/DESCRIPTION’ ... OK
* checking extension type ... Package
* this is package ‘proFIA’ version ‘0.99.0’
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘proFIA’ can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking ‘build’ directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking line endings in C/C++/Fortran sources/headers ... OK
* checking compiled code ... OK
* checking installed files from ‘inst/doc’ ... OK
* checking files in ‘vignettes’ ... OK
* checking examples ... OK
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ...
  Running ‘runTests.R’
 ERROR
Running the tests in ‘tests/runTests.R’ failed.
Last 13 lines of output:
  FAILURE in test_findFIASignal: Error in checkEqualsNumeric(sum(tp$injpeak), 30.31704, tolerance = 10^-4) : 
    Mean relative difference: 0.0002244698

  Test files with failing tests

     test_proFIA.R 
       test_findFIASignal 

  Error in BiocGenerics:::testPackage("proFIA") : 
    unit tests failed for package proFIA
  Execution halted
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes in ‘inst/doc’ ... OK
* checking running R code from vignettes ...
   ‘proFIA-vignette.Rmd’ using ‘UTF-8’ ... OK
 NONE
* checking re-building of vignette outputs ... OK
* checking PDF version of manual ... OK
* DONE

Status: 1 ERROR
See
  ‘/home/hpages/pkgreviews/proFIA.review/github/proFIA.Rcheck/00check.log’
for details.

Please address.

Regards, H.

[bioc-issue-bot]

We only start builds when the Version field in the DESCRIPTION file is incremented. For example, by changing

Version: 0.99.0

to

Version 0.99.1

If you did not intend to start a build, you don't need to do anything. If you did want to start a build, increment the Version: field and try again.

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

eead53d increased version number

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160920045505.html

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160920121358.html

[adelabriere]

Thanks for your reactivity hpages,

The build did not go well because I call library(plasFIA) in my vignette. I still have a circular dependency if I try to find the path of the raw files in the .onLoad of "plasFIA". I just checked if I suppress the .onLoad function the "plasFIA" package can be installed without any dependency. This mean that :

1. I need to change the path to the raw data of plasFIA in the proFIA vignette to be able to use raw data visualisation function. This seems wrong is it the right thing to do ? I also need to change the path in the functions examples which use raw data.

If that's the good thing to do I need to reopen an issue for the plasFIA package first then add the proFIA as an additional package ? I think that's the case but I want to be sure.

Thanks, Alexis

[hpages]

Hi Alexis,

proFIA suggests plasFIA and plasFIA depends on proFIA. This is fine. It is NOT a circular dependency. It just means that proFIA must be installed before plasFIA and that the 2 packages need to be installed before running R CMD build and R CMD check on any of them. It seems that our automated single package builder is having problems dealing with this situation. But it's a problem with our automated single package builder, not with your package.

Please try to bump the version of proFIA and plasFIA in order to trigger new builds. We'll see how it goes. If that still doesn't work, I'll try to install the 2 packages manually on the single package builder.

I'm reporting the issue to the maintainers of the automated single package builder.

Thanks for your patience, H.

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

4b9de55 Removed a useless documentation file 34e7eb8 Type in description

[adelabriere]

Thank for the clarification,

I pushed version 0.99.1 of plasFIA.

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/plasFIA_buildreport_20160921052455.html

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

0cd5acf increased version number to relaunch biocbuild

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160921053049.html

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160921145702.html

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/plasFIA_buildreport_20160921150635.html

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160921151707.html

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

54e6fd6 plasFIA updazted after testing with new version of...

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

4c3beee debuggued the solvent filter and the wrong integra...

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/plasFIA_buildreport_20160922045415.html

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160922045526.html

[adelabriere]

Hello,

Visibly there still an issue, normally proFIA does not use any functionality of mzR and import xcms which use mzR import,. And plasFIA did not find proFIA.

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

7f54664 version 0.99.4 to relaucnh a biocBuild 943ba73 cleaned the bad sources repository

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160926031648.html

[adelabriere]

Hello hpages,

Can you confirm that the issu still come from the BiocBuilder ?

I see somebody in another package tackling the issu by adding an installation of the package in the vignette. I can probably do the same if it is necessary.

Thanks in advance, Alexis.

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS, skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/plasFIA_buildreport_20160927141709.html

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160927143102.html

[hpages]

Hi Alexis,

Lori, the Single Package Builder maintainer, manually kicked new builds for proFIA and plasFIA a few minutes ago, and the results are now available. Please take a look at them. On zin1 (Linux) and morelia (Mac OS), R CMD check fails for proFIA because of a unit test failure. This unit test failure is the one I reported to you 7 days ago. This would need to be addressed. Let's ignore the error on moscato1 (Windows) for now. Thanks!

Regards, H.

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

1039861 prepush for proFIA push 11941cd v 0.99.3

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

d7cb1d3 Corrected the test 04fb463 Cleaned useless source files

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS, skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20160927163733.html

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS, skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/plasFIA_buildreport_20160927164040.html

[adelabriere]

Thanks that's corrected. It was corrected but I did a minor workflow moficiation which was not included in tests. So warnings only now.

I replaced the FALSE by F in the vignette but I did not find any TRUE. I did not manage to register native routines, even if everything seems right in my C code I will keep trying until it passes. The plasFIA warnings is about value in dataset man page which for me is quite strange as there is already a format field..

Thank to you and Lori,

Alexis

[hpages]

Hi Alexis,

Thanks for making these changes. Let's not worry about registering native routines for now. This is indeed good practice but is not a show stopper. It can also be a bit tricky to get right if you're not familiar with it. So, given that we're short in time, I propose that we postpone it. You'll always be able to come back to it once the package is accepted and after the upcoming release.

I'll take another look at proFIA and plasFIA today and will let you know.

Cheers, H.

[hpages]

The packages look good. I only have a few minor comments:

(1) Please use <- (with a space before and after) instead of = for assignments.

(2) Please use TRUE and FALSE instead of T and F.

(3) Please use

system.file(package="plasFIA", "mzML")

instead of

file.path(find.package("plasFIA"), "mzML")

(4) Why is the proFIAset() step of the workflow showing a succession of plots? (Kind of in a slideshow way.) This is not documented. Is this actually intended? If yes, this is not a good feature.

(5) Please do not put the proFIAset example in a dontrun statement. The example actually fails for me with the following error:

Error: 'bplapply' receive data failed:
  X11 fatal IO error: please save work and shut down R

That's actually the reason why examples should not be put in dontrun.

(6) The analyzeAcquisitionFIA example is also in a dontrun statement, which is probably ok. But still, the example should work:

> plasSet <- analyzeAcquisitionFIA(path, ppm=ppm, fracGroup=fracGroup,
                                   ppmgroup=ppmgroup, k=k, parallel=paral)
...

and after a while:

...
Beginning band detection.
21 chromatograms have been used for peak shape determination.
Error: BiocParallel errors
  element index: 1, 2, 3, 4, 5, 6
  first error: X11 fatal IO error: please save work and shut down R

Even worse, a few seconds after I got the above error, my session crashed without me doing anything else at the command line:

> [xcb] Unknown sequence number while processing queue
[xcb] Most likely this is a multi-threaded client and XInitThreads has not been called
[xcb] Aborting, sorry about that.
R: ../../src/xcb_io.c:274: poll_for_event: Assertion `!xcb_xlib_threads_sequence_lost' failed.
Aborted (core dumped)

(7) There is no need to explicitly call show() on your objects in the examples and in the vignette. Just put the symbol of the object on a line by itself like this:

plasSet

instead of

show(plasSet)

(8) The plotRaw() generic is already defined in the xcms package which you depend on (via Imports). Please don't redefine this generic. Instead import plotRaw from xcms and move xcms from Imports to Depends.

(9) Value section in makeDataMatrix man page says:

A proFIAset object with the 'group' field filled.

You probably want to replace 'group' with 'dataMatrix'. Also, following the "official" R terminology, this a "slot", not a "field" (please see man page for group.FIA where "field" is also used instead of "slot").

(10) Typo in "show" method for proFIAset objects:

    > plasSet
    A "proFIAset" object containing  1  classes.
    Data matrix have been created. 
    ...

Should be "Data matrix has been created."

(11) If I call makeDataMatrix() on the plasSet object used in the vignette without calling group.FIA() first, I get this error:

    > plasSet <- makeDataMatrix(plasSet, maxo=F)
    Error in object@groupidx[[i]] : subscript out of bounds

This is obscure for the end user. It should fail with an informative error message.

(12) Same thing if I try to call plotEICs():

    > plotEICs(plasSet,mz=plasMols[4,"mass_M+H"])
    Error in object@group[, "mzMin"] : no 'dimnames' attribute for array

(13) Also if I try to do fillPeaks.WKNN() without having performed the previous steps first:

    > plasSet <- fillPeaks.WKNN(plasSet, k=k)
    Error in object@groupidx[[i]] : subscript out of bounds
    In addition: Warning message:
    In .local(object, ...) :
      It is strongly discouraged to do 1-nearest neighbour.

(14) The following comment in the vignette (fillPeaks.WKNN step)

    # since there is only 1 replicate, k is set to 2
    k=1

does not reflect what the code does. What is correct, the comment or the code? Note that using k=1 gives a warning:

    > plasSet <- fillPeaks.WKNN(plasSet, k=k)
    700 variables could not be imputated.
    Warning message:
    In .local(object, ...) :
      It is strongly discouraged to do 1-nearest neighbour.

Not surprisingly, using k=1 when running the whole workflow with analyzeAcquisitionFIA also produces this warning.

(15) Why do you need to define and export the noiseEstimation class? Where is it used?

(16) Why do you have a man page for a fitModel generic and method? There is no such function available. This man page actually contains an example for plotNoise, not for fitModel.

Thanks, H.

[hpages]

and also:

(17) The estimateNoiseListFiles, determiningSizePeak.Geom, exportVariableMetadata, findFIASignal, and getInjectionPeak man pages need examples.

(18) Why do you have man pages for the compareClasses and exportSampleMetadata generics and methods? There are no such functions available.

(19) Please do not put the exportDataMatrix example in a dontrun statement.

(20) It's ExpressionSet, not expressionSet (R is case-sensitive).

(21) The term "Visualising" seems inappropriate in that context:

    #Visualising the raw data
    eset=exportExpressionSet(plasSet)
    show(eset)

Here too:

    #Visualising the raw data
    ptable=exportPeakTable(plasSet)
    show(ptable)

(22) In the getDataMatrix man page:

     ...: Supplementary arguemnts to be passed to makeDataMatrix if the
          matrix have not been created.

Should be: if the matrix has not been created Typo in "arguemnts" The supplementary arguments are actually not passed to makeDataMatrix if the matrix has not been created yet:

> selectMethod("getDataMatrix", "proFIAset")
Method Definition:

function (object, ...) 
{
    if (nrow(object@dataMatrix) == 0) {
        object = makeDataMatrix(object)
        object@step = "Matrix_created"
    }
    return(object@dataMatrix)
}
<environment: namespace:proFIA>

Also no need to do object@step = "Matrix_created" here. makeDataMatrix() is already taking care of this.

(23) Incomplete \value section in getGroupMatrix man page:

Value:

     a matrix with groups as rows and informations on group as columns.
     See

(24) Finally, I just realize now that you actually have a bunch of accessors to extract things from a proFIAset object: getDataMatrix, getGroupMatrix, getInjectionPeaks, getPeaks, and getPhenoClasses. Please document them all together and in the same man page as the proFIAset class. It's the natural place for that. It makes them much easier to find. Also the "R way" is to not use the get prefix for accessors, so the same name can be used for the getter and the setter. This is considered good practice, even if you're not providing setters. So dataMatrix, groupMatrix, injectionPeaks, peaks, and phenoClasses.

Thanks, H.

[adelabriere]

thank for your feedback, I'll correct this.

4) The succession of slideshow is showing the result of the processing done on individual files.

5 and 6 are visibly error of BiocParallel, which I did not manage to reproduce. I'll just remove the parallel processing in examples. But that's woorying.

8) Okay I'll do that.

9-13) I'll add soem safeguard to ensure that the steps are called in the right order.

14) It's a mistake the code is correct, I'll correct it.

15) I shloud not defini it, it's just that I needed to have a noiseEstimation somewhere and an object was more convenient. I won't export it.

16) Mistake probably. I'll hide everything about the noise Estimator object. I don't want people to mess with it.

I'll correct everything as fast as I can, a big thanks for your quick feedback !

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

2ce6da6 form modifications

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR, WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/proFIA_buildreport_20161005180007.html

[hpages]

Hi Alexis,

I see some progress, but many things have not been addressed or have been addressed only partially. Using the original issue numbers:

(1) There are still many places in your man pages and vignette where you use = for assignment. Please make sure to use <- everywhere for assignment, with a space before and after the arrow.

(3) There are still places in your man pages where you use file.path(find.package(...), ...) instead of system.file(...).

(4) The proFIAset() function still plots the "Noise variance estimation and fitted model" by default despite its graphical argument being FALSE by default.

(5) Thanks for fixing the proFIAset example but it's still in a dontrun statement. Please do not put it in a dontrun statement.

(6) The analyzeAcquisitionFIA example is still broken but with a different error. The new error is:

plasSet <- analyzeAcquisitionFIA(path,ppm=ppm,fracGroup=fracGroup,ppmgroup=ppmgroup,k=k,parallel=paral)
...
Step 2 : Peak Grouping. Area will be used.
A mass interval of 0.0307m/z will be used for the density estimation.
0 :0 10 :56 20 :139 30 :206 40 :281 50 :374 50 :458 60 :521 70 :562 80 :598 90 :628 
 628  groups have been done .
Step 3 : Missing values imputation.
Error in .local(object, ...) : 
  Impossible to perform fillPeaks without data matrix, use makeDataMatrix.

(9) 'dataMatrix' is a slot, not a field. S4 objects have slots, data frames have fields (or variables).

(10) Same "have/has" error for Grouping:

> plasSet
A "proFIAset" object containing  1  classes.
Grouping have been done between samples. 

(11,12,13) Please use consistent error messages:

> plasSet <- makeDataMatrix(plasSet, maxo=F)
Error in .local(object, ...) : 
  Peaks needs to be grouped before creating the data Matrix
> plotEICs(plasSet,mz=plasMols[4,"mass_M+H"])
Error in .local(object, ...) : 
  Groups needs to be created to call plotEICs.

A good practice is to achieve consistency by sharing as much code as possible across functions (this is the "DRY principle" in software engineering). Also, for the sake of making the error message as useful as possible, why not have the error message mention the name of the function that needs to be called in order to group the peaks? Something like:

Peaks needs to be grouped before creating the data Matrix. See ?group.FIA

(14) I still get the warning. Why would the vignette do something that is "strongly discouraged"?

(17) Thanks for adding examples to these man pages. Please don't put estimateNoiseListFiles example in a dontrun statement. Also please don't use proFIA::: when calling estimateNoiseListFiles().

(24) Please export the generics for all your accessors (right now you're only exporting the methods). You will also need to have aliases for these generics in the man page of the proFIAset class (e.g. \alias{dataMatrix}) so the user can access the man page with just ?dataMatrix (right now this doesn't work). Also please add an example section showing how to use these accessors.

New problems:

(25) Here is what the description section of the proFIAset class looks like when I open the man page in a terminal (with class?proFIAset):

  Description:

     An S4 class to represent an FIA experiments.

     #' Extract the injection peaks from a proFIAset object #' #'
     Extract all the regressed peak table from a proFIAset object. #'
     #' @export #' @param object A proFIAset object. #' @return a list
     with the injection peak correspoing to each sample. #' @aliases
     injectionPeaks injectionPeaks,proFIAset-method #' @seealso
     ‘proFIAset-class’ to see the fieds on the peaks table. #'
     @examples #' if(require("plasFIA")) #' data(plasSet) #'
     lInjPeak=injectionPeaks(plasSet) #'

Looks like roxygen got confused. Probably because of some formatting problem in your R source files.

(26) Calling new() on the name of a class only (i.e. with no other arguments) should always return a valid object:

> aa <- new("proFIAset")
> validObject(aa)
Error in validObject(aa) : 
  invalid class “proFIAset” object: invalid object for slot "noiseEstimation" in class "proFIAset": got class "NULL", should be or extend class "noiseEstimation

Please adjust the prototype of the proFIAset class so that new("proFIAset") returns a valid object.

(27) Please remove vignettes/proFIA-vignette.html from the package source tree. The HTML (or PDF) version of the vignette doesn't belong to the package source tree: it is automatically generated from vignettes/proFIA-vignette.Rmd and included in the source tarball by R CMD build.

Thanks, H.

[adelabriere]

Hello,

Thank for yout feedback, I will push all the required modifications today,

You got the issue in fillPeaks because the data which are used in the vignettes are too small, but to stay under 5 minutes of processing I can't add more files. So I just got 2 files by classes which is not supposed to be the case in any real acquisition.

It is a bad pratice in a general case, but on this toy dataset it is the best thing to do. I want it to stay as warnings because in the general case it is wrong.

Regards, Alexis