CoRegFlux - Githubissues

ptrebulle commented 5 years ago

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

Repository: https://github.com/i3bionet/CoRegFlux

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] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[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 Git (optionally via GitHub).

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

bioc-issue-bot commented 5 years ago

Hi @ptrebulle

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: CoRegFlux
Type: Package
Title: CoRegFlux
Version: 0.99.0
Author: Pauline Trébulle, Daniel Trejo-Banos, Mohamed Elati
Maintainer: Pauline Trébulle <coregflux@gmail.com>
Description: CoRegFlux aims at providing tools to integrate reverse engineered gene regulatory networks and gene-expression into metabolic models to improve prediction of phenotypes, both for metabolic engineering, through transcription factor or gene (TF) knock-out or overexpression in various conditions as well as to improve our understanding of the interactions and cell inner-working.
License: GPL-3
Encoding: UTF-8
LazyData: true
Imports:
    CoRegNet,
    sybil
RoxygenNote: 6.1.1
Suggests: 
    glpkAPI,
    testthat,
    knitr,
    rmarkdown,
    digest,
    R.cache,
    ggplot2,
    plyr,
    igraph,
    methods 
biocViews:GeneRegulation,Network,SystemsBiology,GeneExpression,Transcription,GenePrediction
VignetteBuilder: knitr

bioc-issue-bot commented 5 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

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 commented 5 years ago

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 build report for more details.

bioc-issue-bot commented 5 years ago

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

26a4af9 Update DESCRIPTION f5ca1da Update DESCRIPTION 4fc9546 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

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 build report for more details.

bioc-issue-bot commented 5 years ago

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

c840667 Update DESCRIPTION and reduce the size of SC_EXP_D...

bioc-issue-bot commented 5 years ago

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 build report for more details.

bioc-issue-bot commented 5 years ago

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

fb6fbff Update CoregfluxFunctions e4fae77 Version bumped

bioc-issue-bot commented 5 years ago

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 build report for more details.

bioc-issue-bot commented 5 years ago

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

14835c3 Update data size 6494056 Update CoregluxFunctions example 97cdf87 Update DESCRIPTION (version bump + depends)

bioc-issue-bot commented 5 years ago

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 build report for more details.

ptrebulle commented 5 years ago

Dear @Kayla-Morrell, I am currently working on fixing the issues highlighted in the build report but I am not sure about what to do regarding the error message "dependency glpkAPI is not available for package CoRegFlux" when constructed under Linux and OS X . I moved glpkAPI from Suggests to Imports in the Description file previously but the problem remains. Do you have any idea to help me solve this issue ? Additionnaly, for the Windows build, I have warnings regarding the check time, is there any way to reduce it without decreasing the number of evaluation and examples in the vignette ? Thanks you in advance for your help !

lshep commented 5 years ago

I see that glpkAPI is a CRAN package so it should be a suitable dependency - this is on our end. I'll look into why it cannot be installed on the other platforms. @Kayla-Morrell will still be in charge of your review and can assist you with any other questions/concerns.

bioc-issue-bot commented 5 years ago

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

3022788 Update sybil default solver fb678b3 Update Description imports - trigger built

bioc-issue-bot commented 5 years ago

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, TIMEOUT". 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 build report for more details.

ptrebulle commented 5 years ago

Thank you @lshep ! I noticed that the glpkAPI package asks for some system requirements (GLPK (>= 4.42)) which might be the source of the problem ? In my last commit, I changed the solver to another one (with no system requirement) in the vignette to assess if it can facilitate the building of the package on Linux and OS X and, while it works, it seems to greatly slow down the process and to be less efficient in solving the simulations. I revert back to glpkAPI without triggering a new built. Let me know if there is anything I can do to fix this on my side.

lshep commented 5 years ago

I believe we have installed the latest version of GLPK - you can try adding the version back in with a version bump for your package.

bioc-issue-bot commented 5 years ago

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

ce7d2ac Version bump

bioc-issue-bot commented 5 years ago

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 build report for more details.

bioc-issue-bot commented 5 years ago

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

e97335c Update DESCRIPTION

bioc-issue-bot commented 5 years ago

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". 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

9861d24 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

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". 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

b56dd92 Update Simulation example and DESCRIPTION

bioc-issue-bot commented 5 years ago

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". 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

74e180b Update man and DESCRIPTION

bioc-issue-bot commented 5 years ago

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". 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

98cccba Reduce time to run test 847cdc3 Add data to decrease running time 4100be8 Update Version 07fd260 add PredictedGeneState

bioc-issue-bot commented 5 years ago

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". 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

9863130 Update examples to reduce check time

bioc-issue-bot commented 5 years ago

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: "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 build report for more details.

bioc-issue-bot commented 5 years ago

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

0e29060 Update description

bioc-issue-bot commented 5 years ago

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 build report for more details.

Kayla-Morrell commented 5 years ago

Hi @ptrebulle,

Thank you for submitting to Bioconductor. Please see the initial review of your package below:

DESCRIPTION

Good description of the package.

[ ] REQUIRED: set LazyData to FALSE. For packages with data, LazyData set to TRUE slows down the loading of the package.
[ ] SUGGESTION: Include a top-level README file that includes installation steps for the system requirement GLPK so that users can be sure they have it installed properly.

NAMESPACE

[ ] SUGGESTION: @importFrom() is prefered over importing the whole package. It depends on how many functions are used from that package. I discuss this more in the R files section.

data

Looks good, documentation needs some work but this will be addressed in the R files and man pages sections.

vignette

[ ] SUGGESTION: Rename the vignette file to "CoRegFlux.Rmd" so that the user doesn't have to remember to use vignette("coregflux") to access the vignette.
[ ] SUGGESTION: A table of contents is strongly encouraged.
[ ] REQUIRED: In the "Introduction", provide an explanation for the image.
[ ] REQUIRED: Include an "Installation" section (either before or after Introduction) to show the user how to install and load the package from Bioconductor.

Install:

if(!requireNamespace("BiocManager", quietly = TRUE))
    install.packages("BiocManager")
BiocManager::install("CoRegFlux")

Load:

library(CoRegFlux)

[ ] REQUIRED: In the "Data requirement" section, provide a sentence that explains that all of these data sets are provided in the package if they don't have them (I'm assuming all of these data sets are provided...).
[ ] REQUIRED: Switch around the layout of the vignette. The descriptions section you have after the examples are really great but should be incorporated within the examples, that way the user understands what is going on as they work through the vignette. The vignette should explain an idea/function and then demonstrate it with code. My suggested layout is below, but please feel free to change it depending on how you see fit:

Title Description Installation Introduction Data requirement Quick user guide Computing Influence using CoRegNet package function 1 Predict gene state/gene expression level from a condition specific experiment using a linar model Simulation description (from below 8.)
1. Simulate a dFBA over time (here 20h) without constraint I edited this title to include an 'a' before dFBA like the other titles Contraining the model description (from below 8.)
2. Simulate a dFBA with gene expression as a constraint
3. Simulate a dFBA with TF knock-out (KO) while constraining the model with gene expression
4. Simulate a dFBA with TF over-expression (OV) while constraining the model with gene expression
5. Simulate a dFBA with gene(s) knock-out or over-expression simulation while constraining the model with gene expression
6. Constrain the model according to gene expression, TF KO or OV, gene KO or OV to run various FBA using sybil I edited this title so that 'sybil' was indicated as a package, similar to what you do in the beginning with 'CoRegNet' From observations to fluxes description (from below 8.)
7. Adjusting the fluxes bounds based on observed growth rates, and visualized its effects on metabolic genes Calibration: identifying the softplus parameter using baysian optimization description (from below 8.)
[ ] SUGGESTION: For "8. Adjusting the fluxes bounds based on observed growth rates, and visualized its effects on metabolic genes", when evaluating "ODcurveToMetCurve" I get a message about the R.cache package needing to create a directory that will hold cache files. You may want to advise the user as to what they should do here.
[ ] SUGGESTION: Under "Simulation" you discuss how to access gprRules the user can use the sybil package. I would include library(sybil) in the code because if the user just runs the code you provide, an error is produced.
[ ] REQUIRED: I get an ERROR when evaluating the section "Calibration: identifying the softplus parameter using baysian optimization". I'm not sure where exactly the error comes from, but this is the output I get:

Adjusting bounds of metabolic model
elapsed = 0.23  Round = 1   p = 10.0000 Value = 1.7692 
Error in if ((min(X) < 0) | (max(X) > 1) | ((max(X) - min(X)) <= 0.5)) { : 
  missing value where TRUE/FALSE needed

[ ] REQUIRED: Include a section with SessionInfo(), this should be at the very end of the vignette.

R files

[ ] SUGGESTION: In all documentation, you can use @importFrom and list all the function you use from sybil and CoRegNet. Then anywhere in the code where you use sybil:: or CoRegNet:: won't be needed.
[ ] REQUIRED: Any function that isn't being exported should have @keywords internal in the documentation so that a man page isn't created. Man pages should only be available for functions that are being exported.
[ ] REQUIRED: It's my understanding that if a function is listed as "See Also" than it should be exported or else the user won't see the function. Double check that the correct functions are being exported to the user.

CoregfluxFunctions.R

[ ] REQUIRED: clean up code in predict_linear_model_influece() (starting at line 461) by removing unneeded comments (mainly the if/else statement).
[ ] REQUIRED: Line 234, 339, and 386, use either seq_len() or seq_along() instead of 1:...

DFBAsimulationFunctions.R

[ ] REQUIRED: Line 236, use either seq_len() or seq_along() instead of 1:...

FVAFunctions.R

[ ] REQUIRED: Line 123 and 133, use either seq_len() or seq_along() instead of 1:...

ODToGenes.R

[ ] SUGGESTION: I would add igraph to Imports for your DESCRIPTION file and use @importFrom igraph and list all the functions you use. This will help clean up your code of all the igraph::... instances.

data.R

[ ] REQUIRED: Add the @format tag to the "SC_GRN_1" data set. It looks like you include the format in the description but it would be better if it was explicit like the other data sets.
[ ] REQUIRED: Add \describe{} where possible.
[ ] REQUIRED: Be sure to add @source tag to provide details of where you got the data from (often a \url{}).
See http://r-pkgs.had.co.nz/data.html "Documenting datasets" for examples

man pages I only reviewed the man pages for the functions that are being exported since these are the ones that the user will end up seeing. The others ones should be taken care of once the tag @keywords internal is added.

[ ] REQUIRED: For all man pages, be sure to indicate in code when talking about functions. You can use the `function_name()` to help distinguish this in text. An example of this is in "CoregfluxFunctions.R", the description of coregflux_static() should have "coregflux_static() uses...", in the param predicted_gene_expression should have "...given by predict_linear_model_influence(), etc.

ODCurveToFluxCurves.Rd

[ ] REQUIRED: Improve the Description of the file

ODCurveToMetabolicGeneCurves.Rd

[ ] REQUIRED: Improve the Description of the file
[ ] REQUIRED: Add a period after Optional for "singlePointFluxEstimate"
[ ] REQUIRED: The return value says "Metabolic genes curves" but when I run the examples no curves are produced. Double check that this code is correct and/or this is the correct return value.

ODcurveToMetCurve.Rd

[ ] REQUIRED: Fix the title.

ODtoflux.Rd

[ ] REQUIRED: Fix the title.

PredictedGeneState.Rd

[ ] REQUIRED: Fix the title.

SC_EXP_DATA.Rd

[ ] REQUIRED: Fix the title.

SC_GRN_1.Rd

[ ] REQUIRED: Shorten the title. Most of this is repeated in the description.

SC_Test_data.Rd

[ ] REQUIRED: Fix the title.

SC_experiment_influence.Rd

[ ] REQUIRED: Fix the title.

aliases_SC.Rd

[ ] REQUIRED: Fix the title.

coregflux_static.Rd

[ ] REQUIRED: Edit the \value{}, constraints is misspelled for "model:". Make sure there is a return after "added" so that "softplus_positive:" will be on a new line

get_biomass_flux_position.Rd

[ ] REQUIRED: Add more description to the parameter "biomass_reaction_name".

get_fba_fluxes_from_observarions.Rd

[ ] REQUIRED: For consistency sake, make sure there is a period after Optional for backward_fluxes and forward_fluxes.

get_fva_intervals_from_observations.Rd

[ ] REQUIRED: In Description, write out what fva stands for and then you can use the abbreviation throughout.

iMM904.Rd

[ ] REQUIRED: Fix the title.

predict_linear_model_influence.Rd

[ ] REQUIRED: Edit the arguments model and train_influence. Optional should have a period after it. It is unclear what type these arguments are. Are they TRUE/FALSE?

visMetabolicGeneCurves.Rd

[ ] REQUIRED: Add a period after Optional

miscellaneous

[ ] SUGGESTION: Highly recommend unit tests, they are very helpful with package development and maintenance.

Everything else looks good. Let me know if you have any questions or need clarification on any of the points above. Please comment back here with updates that have been made and when the package is ready for a re-review.

Best, Kayla

bioc-issue-bot commented 5 years ago

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

adeca15 Improved functions documentations fa3d307 Add keyword internal to function not exported 1c15884 Add data to titles in data.R 827827f Add Installation, Figure explanation, SessionInfo ... 9a4ef73 Add info on R.cache directory bb509ac Rewrite iMM904 e93fd9d Reorganize setup vignette 7ec4e26 Update vignette, change layout, add table of conte... d951ac0 Version bumped

bioc-issue-bot commented 5 years ago

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: "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 build report for more details.

bioc-issue-bot commented 5 years ago

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

1983a80 add data(iMM904) in example 1022bea Version bumped

bioc-issue-bot commented 5 years ago

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: "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 build report for more details.

bioc-issue-bot commented 5 years ago

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

e9e2718 Fix data examples c2db681 Version bump + test version R

bioc-issue-bot commented 5 years ago

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". 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

2b113f7 Version bump

bioc-issue-bot commented 5 years ago

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". 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 build report for more details.

ptrebulle commented 5 years ago

Dear @Kayla-Morrell , Thanks for your comments. You will find bellow the changes carried out on the package.

Some precisions on our corrections: CoRegNet and sybil functions are used a lot in the code and will ease the user experience as they probably will use those functions themselves to interrogate their network and genome-scale metabolic model, thus, we think a general import is more relevant here.
Regarding the unit tests: is the number of units tests already provided unsufficient in your opinion ? We used the testthat package to include about a dozen of unit test to ensure that the functions work as expected, which we thought was sufficient to cover the functionnalities of CoRegFlux.

I believe the package is ready for re-review but the last build report had a warning which I am not sure I can fix myself since it is related to the accessibility of http://worldclockapi.com/api/json/utc/now .

DESCRIPTION

[X] REQUIRED: set LazyData to FALSE. For packages with data, LazyData set to TRUE slows down the loading of the package.
[X] SUGGESTION: Include a top-level README file that includes installation steps for the system requirement GLPK so that users can be sure they have it installed properly.

NAMESPACE
[ ] SUGGESTION: @importFrom() is prefered over importing the whole package. It depends on how many functions are used from that package. I discuss this more in the R files section.

data

Looks good, documentation needs some work but this will be addressed in the R files and man pages sections.

vignette
[X] SUGGESTION: Rename the vignette file to "CoRegFlux.Rmd" so that the user doesn't have to remember to use vignette("coregflux") to access the vignette.
[X] SUGGESTION: A table of contents is strongly encouraged.
[X] REQUIRED: In the "Introduction", provide an explanation for the image.
[X] REQUIRED: Include an "Installation" section (either before or after Introduction) to show the user how to install and load the package from Bioconductor. [...]
[X] REQUIRED: In the "Data requirement" section, provide a sentence that explains that all of these data sets are provided in the package if they don't have them (I'm assuming all of these data sets are provided...).
[X] REQUIRED: Switch around the layout of the vignette. The descriptions section you have after the examples are really great but should be incorporated within the examples, that way the user understands what is going on as they work through the vignette. The vignette should explain an idea/function and then demonstrate it with code. My suggested layout is below, but please feel free to change it depending on how you see fit [...]
[X] SUGGESTION: For "8. Adjusting the fluxes bounds based on observed growth rates, and visualized its effects on metabolic genes", when evaluating "ODcurveToMetCurve" I get a message about the R.cache package needing to create a directory that will hold cache files. You may want to advise the user as to what they should do here.
[X] SUGGESTION: Under "Simulation" you discuss how to access gprRules the user can use the sybil package. I would include library(sybil) in the code because if the user just runs the code you provide, an error is produced.
[X] REQUIRED: I get an ERROR when evaluating the section "Calibration: identifying the softplus parameter using baysian optimization". I'm not sure where exactly the error comes from, but this is the output I get: [...]
[X] REQUIRED: Include a section with SessionInfo(), this should be at the very end of the vignette.

R files
[ ] SUGGESTION: In all documentation, you can use @importFrom and list all the function you use from sybil and CoRegNet. Then anywhere in the code where you use sybil:: or CoRegNet:: won't be needed.
[X] REQUIRED: Any function that isn't being exported should have @keywords internal in the documentation so that a man page isn't created. Man pages should only be available for functions that are being exported.
[X] REQUIRED: It's my understanding that if a function is listed as "See Also" than it should be exported or else the user won't see the function. Double check that the correct functions are being exported to the user.

CoregfluxFunctions.R
[X] REQUIRED: clean up code in predict_linear_model_influece() (starting at line 461) by removing unneeded comments (mainly the if/else statement).
[X] REQUIRED: Line 234, 339, and 386, use either seq_len() or seq_along() instead of 1:...

DFBAsimulationFunctions.R
[X] REQUIRED: Line 236, use either seq_len() or seq_along() instead of 1:...

FVAFunctions.R
[X] REQUIRED: Line 123 and 133, use either seq_len() or seq_along() instead of 1:...

ODToGenes.R
[ ] SUGGESTION: I would add igraph to Imports for your DESCRIPTION file and use @importFrom igraph and list all the functions you use. This will help clean up your code of all the igraph::... instances.

data.R
[X] REQUIRED: Add the @format tag to the "SC_GRN_1" data set. It looks like you include the format in the description but it would be better if it was explicit like the other data sets.
[X] REQUIRED: Add \describe{} where possible.
[X] REQUIRED: Be sure to add @source tag to provide details of where you got the data from (often a \url{}).
- See http://r-pkgs.had.co.nz/data.html "Documenting datasets" for examples
man pages I only reviewed the man pages for the functions that are being exported since these are the ones that the user will end up seeing. The others ones should be taken care of once the tag @keywords internal is added.
[X] REQUIRED: For all man pages, be sure to indicate in code when talking about functions. You can use the function_name() to help distinguish this in text. An example of this is in "CoregfluxFunctions.R", the description of coregflux_static() should have "coregflux_static() uses...", in the param predicted_gene_expression should have "...given by predict_linear_model_influence(), etc.

ODCurveToFluxCurves.Rd
[X] REQUIRED: Improve the Description of the file

ODCurveToMetabolicGeneCurves.Rd
[X] REQUIRED: Improve the Description of the file
[X] REQUIRED: Add a period after Optional for "singlePointFluxEstimate"
[X] REQUIRED: The return value says "Metabolic genes curves" but when I run the examples no curves are produced. Double check that this code is correct and/or this is the correct return value.

Clarifications on the return value have been provided

ODcurveToMetCurve.Rd
[X] REQUIRED: Fix the title.

ODtoflux.Rd
[X] REQUIRED: Fix the title.

PredictedGeneState.Rd
[X] REQUIRED: Fix the title.

_SC_EXPDATA.Rd
[X] REQUIRED: Fix the title.

_SC_GRN1.Rd
[X] REQUIRED: Shorten the title. Most of this is repeated in the description.

_SC_Testdata.Rd
[X] REQUIRED: Fix the title.

_SC_experimentinfluence.Rd
[X] REQUIRED: Fix the title.

_aliasesSC.Rd
[X] REQUIRED: Fix the title.

_coregfluxstatic.Rd
[X] REQUIRED: Edit the \value{}, constraints is misspelled for "model:". Make sure there is a return after "added" so that "softplus_positive:" will be on a new line

_get_biomass_fluxposition.Rd
[X] REQUIRED: Add more description to the parameter "biomass_reaction_name".

_get_fba_fluxes_fromobservarions.Rd
[X] REQUIRED: For consistency sake, make sure there is a period after Optional for backward_fluxes and forward_fluxes.

_get_fva_intervals_fromobservations.Rd
[X] REQUIRED: In Description, write out what fva stands for and then you can use the abbreviation throughout.

iMM904.Rd
[X] REQUIRED: Fix the title.

_predict_linear_modelinfluence.Rd
[X] REQUIRED: Edit the arguments model and train_influence. Optional should have a period after it. It is unclear what type these arguments are. Are they TRUE/FALSE?

visMetabolicGeneCurves.Rd
[X] REQUIRED: Add a period after Optional

miscellaneous
[ ] SUGGESTION: Highly recommend unit tests, they are very helpful with package development and maintenance.

Let me know if there is anything I can help with !

bioc-issue-bot commented 5 years ago

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

432e6ea Trigger build

bioc-issue-bot commented 5 years ago

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 build report for more details.

Kayla-Morrell commented 5 years ago

Hello @ptrebulle,

Thank you for the work done on your package. I must have missed the unit tests the first time around. The amount you have is fine so please ignore that comment. I have looked over the changes made and have just a few more requirements/suggestions before accepting your package. See the secondary review below. Comment back here when you are ready for me to look it over again.

vignette

[ ] REQUIRED: In Data Requirement section, the citation does not render correctly.
[ ] REQUIRED: Under User guide - 4.2 Predict gene state/gene expression level from a condition specific experiment using a linear model, there needs to be a data("iMM904") command or PredictedGeneState() won't execute.
[ ] REQUIRED: Under 4.3 Simulations, the code needs to be executable. The user should be able to copy the corresponding commands into an R session and get the same results. Since the Simulation1 variable is defined in the beginning of your vignette in the 'r setup' chunk of R code. The user does not have this code provided in the vignette so if they were to run the command you provide under 4.3 Simulations, they would get an error Error: object 'Simulation1' not found. (You do however define Simulation1 later on in the vignette - section 4.3.1, so maybe this line of code belongs down with that section.)
[ ] SUGGESTION: Under Section 4.4.5 Constraining the model according to gene expression, TF KO or OV, gene KO or OV to run various FBA using sybil, I think this section of code should end with a print out of sol so that the user knows what it looks like (like done in section 4.4).
[ ] REQUIRED: The package "rBayesianOptimization" should be added to the Suggests packages in your DESCRIPTION file.
[ ] CLARIFICATION: We prefer to have all chunks of code to be evaluated. What is the reason to have the code in Section 4.4.5 and Section 4.6 have eval=FALSE?
[ ] REQUIRED: Avoid the use of direct slot access with '@' in the example code. Accessor methods should be created and utilized instead.

R files

[ ] REQUIRED: For data.R, 'SC_EXP_DATA' the '@source' url should not have \url{} around the url. Instead use '<' url '>'.

General Package Development

These notes come from R CMD BiocCheck

[ ] SUGGESTION: If possible, use vapply() instead of sapply(). This was found in ODToGenes:
- line 65
- line 66
- line 69
- line 75
- line 82
- line 116
- line 122
- line 148
- line 184
- line 309
- line 322
- line 332
- line 439
[ ] SUGGESTION: Avoid using 1:..., use seq_len() or seq_along(). Found in files:
- CoregfluxFunctions.R, line 147
- FVAFunctions.R, line 36
- ODToGenes.R, line 485
- ODToGenes.R, line 526

bioc-issue-bot commented 5 years ago

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

d39cf43 correct 1: to seq_len 25557be change sapply into vapply f13daa5 Update corresponding man files f285047 Add rBayesianOptimization to suggest package in De... 2f9b53e Update vignette 1bb4d17 Revert vapply to sapply 7e38636 Trigger build

bioc-issue-bot commented 5 years ago

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: "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 build report for more details.

ptrebulle commented 5 years ago

Hi @Kayla-Morrell , Thanks for your new comments. We updated the code according to them. Regarding the clarification, we used to put eval=FALSE due to a timeout error when submitting the package to Bioconductor to compile and evaluate the vignette. However, as the examples were updated since the first submission, it should not be a problem anymore, thus I turned the eval back to TRUE as suggested. In my last build, I encountered an error during the "Check" step on Linux and MacOS which I think is due to some internet connexion problem. Can you have a look at it and tell me if it is on your side or related to the package ? Once this is fixed, I believe CoRegFlux will be ready for a re-review. Thanks in advance !

[X] REQUIRED: In Data Requirement section, the citation does not render correctly.
[X] REQUIRED: Under User guide - 4.2 Predict gene state/gene expression level from a condition specific experiment using a linear model, there needs to be a data("iMM904") command or PredictedGeneState() won't execute.
[X] REQUIRED:Under 4.3 Simulations, the code needs to be executable. The user should be able to copy the corresponding commands into an R session and get the same results. Since the Simulation1 variable is defined in the beginning of your vignette in the 'r setup' chunk of R code. The user does not have this code provided in the vignette so if they were to run the command you provide under 4.3 Simulations, they would get an error Error: object 'Simulation1' not found. (You do however define Simulation1 later on in the vignette - section 4.3.1, so maybe this line of code belongs down with that section.)
[X] SUGGESTION: Under Section 4.4.5 Constraining the model according to gene expression, TF KO or OV, gene KO or OV to run various FBA using sybil, I think this section of code should end with a print out of sol so that the user knows what it looks like (like done in section 4.4).
[X] REQUIRED: The package "rBayesianOptimization" should be added to the Suggests packages in your DESCRIPTION file.
[X] CLARIFICATION: We prefer to have all chunks of code to be evaluated. What is the reason to have the code in Section 4.4.5 and Section 4.6 have eval=FALSE?
[X] REQUIRED: Avoid the use of direct slot access with '@' in the example code. Accessor methods should be created and utilized instead.

R files

[X] REQUIRED: For data.R, 'SC_EXP_DATA' the '@source' url should not have \url{} around the url. Instead use '<' url '>'.

General Package Development

These notes come from R CMD BiocCheck

[ ] SUGGESTION:

If possible, use vapply() instead of sapply(). This was found in ODToGenes: [...]
[X] SUGGESTION:

Avoid using 1:..., use seq_len() or seq_along(). Found in files:

CoregfluxFunctions.R, line 147 FVAFunctions.R, line 36 ODToGenes.R, line 485 ODToGenes.R, line 526

Kayla-Morrell commented 5 years ago

@ptrebulle

Try another version bump, that should get rid of the errors.

Bioconductor / Contributions

CoRegFlux #975

vignette

R files

General Package Development