Xeva : XEnograft Visualization & Analysis

gangeshberi 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/bhklab/Xeva

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.

spocks commented 5 years ago

hi @lshep I think in the current built, warnings have nothing to do with the package

Warning in readLines(file, skipNul = TRUE) :
  InternetOpenUrl failed: 'The operation timed out'
Error in readLines(file, skipNul = TRUE) : cannot open the connection
Calls: <Anonymous> ... checkDeprecatedPackages -> getAllDeprecatedPkgs -> read.dcf -> readLines
Execution halted

lshep commented 5 years ago

Yes this ERROR is on our end and can be ignored for now - Are you ready for a re-review?

spocks commented 5 years ago

Yes! Please see my comments about previous issues. We have tried our hard to address all the issues. I have posted then again here:

Hi @lshep, thanks for your detailed comments. We have address all the comments and have made substantial changes in the code. The only thing that we can’t change at this moment is switch to SummarizedExperiments. As I have explained later also this is due to dependency on PharmacoGx package. We are working on the new release of PharmacoGx package where we plan to switch to SummarizedExperiments for both the packages. Please find our response to the issues below. We are looking forward to be accepted to Bioconductor.

General

[x] from the build report please fix the coding practices NOTES of using vapply instead of sapply and seq_len() or seq_along() instead of 1:...

We have replaced all sapply and 1:...
[x] We strongly encourage the use of unit tests to test correctness and accuracy of code

We have performed unit tests for some important functions. In future we plan to update it for all necessary functions and will release it on github.
[x] Why include the dontrun in the getExperiment? The exampels run instaneously

We have fixed this issue.
[x] Similarly why the dontrun in setResponse it seems to run fairly efficiently.

We have removed the dontrun from the function.

NEWS

[x] Please remove the ============ at the bottom of the file.

We have fixed this issue.

NAMESPACE

[x] There are a lot of exports please ensure that all of these should be exposed to the user and that some of these should not be internal and only called within other functions

We have made sure that only the required functions are exported. We have also removed several functions from the export.

DESCRIPTION

[x] Please remove LazyData: true in our experience this rarely proves useful and can often slow down installation of the package.

We have fixed this issue.

README

[x] please also include Bioconductor install instructions

We have fixed this issue.

Vignette

[x] I suggest changing the name of the vignette to the name of the package. While this is entirely optional its is much more intuitive for a user to do vignette("Xeva") than try to hunt down the name of the vignette to do vignette("getting-started")

We have fixed this issue.
[x] Its fine that you start with an object to use for the example in the vignette but considering the object is a specialized class you have created at least reference the functions that the user can look at to create their own.

In the vignette, we have included a section to create object with an example and required data is also included.

MAN

[x] braca.Rd please include source information and more detail for this dataset.

We have fixed this issue.
[x] ALL man documents could use more detail. When looking at the documentation when investigating the dontruns, I can't figure out what most of the arugments are in setResponse and getExperiment especially ones where you just indicate default used. There should be explaintation of what these arguments are and how they are used; if they are not used then they should be removed. This applies to all man files/functions

Fixed. We have rechecked all the man documents to make sure all arguments are described.
[x] Please include an example for your class constructor - I currently have no idea how to create an object of the class to be able to use in the vignette pipeline since you provided the object already in your class format.

We have included a detailed example in the vignette.

R code General

[x] We recommend the use of BiocParallel over parallel. Please explain if not going to update
[x] ExpressionSets are outdated and have been replaced with SummarizedExperiments. Please update all instances unless you can provide very good reasoning for not.

For both of these issues, at the moment it is not possible to switch. This is because of the dependency on another Bioconductor package from our group, PharmacoGx. We are working on the new version of PharmacoGx, where we plan to implement these changes in both Xeva and PharmacoGx.
[x] Remove commented out code. This should be added only when live

We have fixed this issue.

XevaSet_Class.R

[x] I have no idea how to create an object of your class

We have included an example in the function documentation. Furthermore a detailed example is also included in the Vignette under the section “Creating new Xeva object”.

access_expressionData_functions.R

[x] expressionsets are outdated. update to use a summarizedExperiment

This is because of the dependency on another Bioconductor package from our group, PharmacoGx. We are working on the new version of PharmacoGx, where we plan to implement these changes in both Xeva and PharmacoGx.
[x] we don't encourage half implemented code - remove .batch2DataFram and batch_SummarizedMolProfiles and include only when code is live. (you should be able to git track this)

We have reviewed the code and made sure no part of the code is half implemented.

access_slot_drug.R

[x] Should there be any sort of validation for the replacement/setting of drugInfo slot? Does the data.frame need a particular format?

We have fixed this issue.

access_slot_expDesign.R

[x] remove commented out code until live

We have fixed this issue.
[x] What about the case that both are not null?

In such case batch will take precedent. Have included it in the details.
[x] What happens in getBatchFormatted, when in the else? can all values still be the default NULL or should something be defined?

Fixed in code. Now in such case it returns error.

access_slot_experiment.R

[x] (optional) seems like the print method for the object class should be with the class definition in XevaSet_Class.R.

We have fixed this issue.

checkModel.R

[x] Again optional but perhaps this is better placed with the XevaSet_Class.R code.

The position of the code has been changed.

creat_Experiment_slot.R

[x] modelClassS4Vars() where did these hard coded values come from?

They are from Meehan, et al. Cancer research 77.21 (2017). Have added a note in the function also

create_drug_slot.R

[x] could this function be combined with a different file to reduce the number of different files? What is the point of this function?

The position of the code has been changed.

create_modToBiobaseMap_slot.R/create_sensitivity_slot.R

[x] Again completely optional but if these are validity methods (which it seems like) then it is probably best to include in the file that has the class definiation.

The position of the code has been changed.

lshep commented 5 years ago

Clarification: You say you can't switch to SummarizedExperiments because of the dependency on the PharmacoGx package however this dependency is not listed in the Description or the namespace? In fact, it is not referenced at all in the documentation or the vignette. How then is this package dependent on PharamcoGx? If this can be used in conjunction with the PharamcoGx package in a pipeline/workflow sort of way then this should be emphasized in the vignette and perhaps data from that package should be used instead of providing additional? I see now that there are a few R files that say copy from PharmacoGx, copying from another package even your own is not allowed and never encouraged. If these functions are included in PharmacoGx then you should list PharmacoGx in the NAMESPACE and DESCRIPTION and import the necessary function to use in your package. Please remove this code and import the necessary functions.