TDbasedUFEadv

[tagtag]

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

• Repository: https://github.com/tagtag/TDbasedUFEadv

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

• I understand that by submitting my package to Bioconductor,
  the package source and all review commentary are visible to the
  general public.

• I have read the Bioconductor Package Submission
  instructions. My package is consistent with the Bioconductor
  Package Guidelines.

• I understand Bioconductor Package Naming Policy and acknowledge
  Bioconductor may retain use of package name.

• 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.

• My package addresses statistical or bioinformatic issues related
  to the analysis and comprehension of high throughput genomic data.

• 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 Bioconductor code of conduct and
  agree to abide by it.

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

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

For questions/help about the submission process, including questions about
the output of the automatic reports generated by the SPB (Single Package
Builder), please use the #package-submission channel of our Community Slack.
Follow the link on the home page of the Bioconductor website to sign up.

[bioc-issue-bot]

Hi @tagtag

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: TDbasedUFEadv
Type: Package
Title: TDbasedUFEadv
Version: 0.99.0
Authors@R: 
    person("Taguchi", "Y-h.", ,
    "tag@granular.com", role = c("aut", "cre"),
    comment = c(ORCID  = "0000-0003-0867-8986"))
Description: 
    This is a comprehensive package to perform
    Tensor decomposition based unsupervised feature extraction.
    It can perform unsupervised feature extraction.
    It uses tensor decompission.
    It is  applicable to gene expression, DNA methylation, and 
    histone modification etc.
    It can perform multiomics analysis.
    It is also applicable to single cell omics data sets.
biocViews: 
    GeneExpression, 
    FeatureExtraction, 
    MethylationArray,
    SingleCell    
License: GPL-3
Encoding: UTF-8
Imports: 
    TDbasedUFE,
    Biobase,
    RTCGA,
    RTCGA.rnaseq,
    RTCGA.clinical,
    utils,
    GenomicRanges,
    rTensor,
    BiocStyle,
    MOFAdata,
    methods,
    graphics,
    stats,
    RITAN,
    STRINGdb,
    enrichR
RoxygenNote: 7.2.3
Suggests: 
    knitr,
    rmarkdown,
    testthat (>= 3.0.0)
VignetteBuilder: knitr
Config/testthat/edition: 3

[vjcitn]

On initial testing I am seeing

--- re-building ‘Enrichment.Rmd’ using rmarkdown
Quitting from lines 41-68 (Enrichment.Rmd) 
Error: processing vignette 'Enrichment.Rmd' failed with diagnostics:
R: geometry does not contain image `/tmp/RtmplX2cjc/Rbuild5cdc763a79ea2/TDbasedUFEadv/vignettes/Enrichment_files/figure-html/unnamed-chunk-2-1.png' @ warning/attribute.c/GetImageBoundingBox/247

[vjcitn]

I don't know why I encountered that with R CMD check, but I will assume it will be remedied if it occurs on the build system. I noted many spelling errors in vignettes; perhaps you can use an automated spelling analyzer to deal with this. The QuickStart has we continue to type 1 and press enter unril we can see the nineth one as -- this approach to exploring data is outmoded. The package passes check so I will pass it to review, but I would suggest you have a look at a) curatedTCGAData as a way of compactly managing many TCGA tumors, and b) shiny as a way of managing user interactions with a large number of results. Developer mentoring may be available to discuss how a shiny app might help your users.

[tagtag]

@vjcitn Thank you very much for your comments. The error did not occur in our system. In the case we cannot correct it, I will ask you. I will check the spelling. I will consider the usage of curatedTCGAData as well as shiny.

[bioc-issue-bot]

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

IMPORTANT: Please read this documentation for setting
up remotes to push to git.bioconductor.org. It is required to push a
version bump to git.bioconductor.org to trigger a new build.

Bioconductor utilized your github ssh-keys for git.bioconductor.org
access. To manage keys and future access you may want to active your
Bioconductor Git Credentials Account

[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: "ERROR, skipped".
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. This link will be active
for 21 days.

Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/TDbasedUFEadv to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

[tagtag]

@lazappi Although @vjcitn advised me to use curatedTCGAData instead of RTCGA, I found that it is not suitable for us to employ curatedTCGAData. In our vignettes QuickStart, we used all TCGA cancer data set from RTCGA.rnaseq. If we try to do the same by curatedTCGAData, we need to perform
all <- curatedTCGAData(
"*", "RNASeq2Gene", version = "2.0.1", dry.run = FALSE
)
and curatedTCGAData started to download data. Download did not finish even after one hour passed. In contrast to curatedTCGAData, RTCGA.rnaseq includes data within it, thus no need to download. Then I gave up the usage of curatedTCGAData. If you think that I am wrong, I am glad if you can advise me the correct way of using curatedTCUAData.

[tagtag]

@lazappi @vjcitn also advised me to use shiny instead of menu. However, I found that bioconductor can only accept shiny as a whole package, not a part of package.
https://contributions.bioconductor.org/shiny.html
TDbasedUFEadv cannot be shiny app as a whole, since only a part of package is interactive. Thus, the usage of shiny as a part of TDbasedUFE is impossible. If you think that I am wrong, I am glad if you can advise me the correct way of using shiny in TDbasedUFEadv.

[lazappi]

• I'm not familiar with the TCGA packages, can you explain the difference between {curatedTCGAData} and {RTCGA}?
• What do you mean by "can only accept shiny as a whole package"? I couldn't see a reference to that in the documentation you linked to and if fact I think it requires that a shiny app functions as a normal package with only the interactive part being included in the app.
• Please address the errors from the build system before I can review the package.

[vjcitn]

Let's leave curatedTCGAData to one side for now; @LiNk-NY may wish to comment at some point but it is not urgent. This comment in the linked element of contributors.bioconductor.org [Shiny apps](https://shiny.rstudio.com/) can be submitted to Bioconductor as software packages. seems misleading to me. Packages may include shiny apps but the apps are supposed to be testable functions. I will address this with the maintainers of the guidelines. I think it is overbroad wording.

[tagtag]

@vjcitn @lazappi Thanks for your valuable comments. Yes, I postpone the comments about curatedTCGA at the moment. As for Shiny, I am waiting for the feedback from you or maintainers to know if menu functions I implemented can be replaced with Shiny apps included in individual packages, not as an independent package that includes only Shiny apps. As for errors, I will check it but I was also asked to check spelling. After spell check I will also address build error (actually speaking, we do not have this errors in our side but I will try to fix it).

[tagtag]

@vjcitn @lazappi Thanks for your valuable comments. Yes, I postpone the comments about curatedTCGA at the moment. As for Shiny, I am waiting for the feedback from you or maintainers to know if menu faunctions can be replaced with Shiny apps included in individual packages, not as an independent package that includes only Shiny apps. As for errors, I will check it but I was also asked to check spelling. After spell check I will also adress build error (actually speaking, we do not have this errors in outr side but I will try to fix it).

[LiNk-NY]

all <- curatedTCGAData(
"*", "RNASeq2Gene", version = "2.0.1", dry.run = FALSE
)
and curatedTCGAData started to download data.

Hi Taguchi, @tagtag

I noticed that the assays argument (RNASeq2Gene) would return both RNASeq2Gene and RNASeq2GeneNorm. Was this the intention? Perhaps, but in either case, a sole RNASeq2Gene input should only return RNASeq2Gene type of assays. I have updated curatedTCGAData to work this way (see version 1.21.2). If you do intend to work with both type of assays, the input should be RNASeq2Gene*.

Note that curatedTCGAData software is lightweight and only downloads the data requested. If you are requesting two assay types from all cancer types, that would require the download of 69 assay files and 66 metadata files (which include colData and metadata) versus 33 assay files and 66 metadata files when requesting only RNASeq2Gene assays.

The download time would also depend on your connection speed relative to the hosting location.

[tagtag]

@LiNk-NY I know that my code download two assays, (thus in total 69 files), and they can be 33 files. However, even if 33 files are download, it took so long time (It is not lightweighted if we need to download as many as 33 files). I know that it depends upon connection speed, too, but I dislike to use something related to connection speed in my package, but something store data in local PC like RTCGA. I am glad if you can consider the situation here. Is it acceptable? > @vjcitn @lazappi

[LiNk-NY]

The typical use case for curatedTCGAData involves a single cancer type and is quite lightweight and integrative.

Is it acceptable?

If I may, I think the use of RTCGA.rnaseq is acceptable. I would suggest making it clear in the vignette that you are using old release TCGA data from 20151101 (based on the package version number and as shown on the website https://rtcga.github.io/RTCGA/).

[tagtag]

@LiNk-NY Thanks. I will mention about it in Vignettes. Since I have used RTCGA only for demonstration purpose in Vignettes, it does not matter that it is an old package.

[bioc-issue-bot]

Received a valid push on git.bioconductor.org; starting a build for commit id: 2092c836a10f48882c5127c68a74f741a249a2ec

[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, ERROR, skipped".
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. This link will be active
for 21 days.

Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/TDbasedUFEadv to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

[bioc-issue-bot]

Received a valid push on git.bioconductor.org; starting a build for commit id: a886ac19242502cc91fb8de1b84a01d3399ac1a1

[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, ERROR, skipped".
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. This link will be active
for 21 days.

Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/TDbasedUFEadv to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

[tagtag]

The build error caused seems to be due to system errors, not because of my package, since it says

Could not find executable pandoc-citeproc

This means that pandoc is missing in the system that tries to build.
I will postpone the effort to address other issues until this problem is fixed.

[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".
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. This link will be active
for 21 days.

Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/TDbasedUFEadv to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

[tagtag]

Dear @lazappi ,

Now Single Package Builder problem was fixed and the present version, 0.99.15, has gotten "OK" flag. I am glad if you can comment the present version when you have time.

Yours, tag.

[lazappi]

• 🚨 Please use selective imports using importFrom instead of importing all with import

I did.

There are still quite a few normal imports. It is ok if these are unavoidable but I don't think this is the case here. I think some of them ({BiocStyle}, {RTCGA},...) could be made suggested dependencies as they are only used for the vignette/examples, not the core package functionality.

The NEWS file

• ⚠️ Please keep the NEWS file up to date with the latest changes

I did.

I can't see any changes to the NEWS.md file. I just see:

# TDbasedUFEadv 0.99.9

* Addressing many comments from reviewers. 

Package data

• 🚨 The data in inst/extdata/ should be documented in inst/script

I did.

It's great you added a link to where to get the data but this should also include some information about what the data is, file formats, information it contains, what it is used for etc.

README

• 🚨 The README looks incomplete, please update to fill in the missing content

Although I added some, I am not sure what to add.

I think this is ok now

• ⚠️ It was not clear to me which order to look at the vignettes. I expected to start with TDbasedUFEadv.Rmd but that only convered technical detail, not any information about how to use the package. I suggest renaming/reorganising the vignettes to make this clearer.

It is renamed as "Explanation_of_TDbasedUFEadv"

I think this is better now, it is clear that one vignette explains the method and the other one explains how to use the package. Just double check you have updated the names everywhere, I think I still saw some references to "Quickstart"

• 🟢 Usually it is better to use library(package) rather than require(package). I also find it easier to follow the code if the library calls are in a separate chunk at the start.

I did.

The examples still use require().

Unit tests

• 🚨 Please add unit tests for all exported functions. There is some code in the tests file currently by no actual expectation checks

I did.

There are still no actual tests here. As you are using the {testthat} package your tests should be wrapped in the test_that() function. Please refer to the {testthat} documentation or the testing chapter of the R packages book.

• ⚠️ I found the SummarizedExperimentTensorRect class a bit unclear in a few ways

  • Given the name I expected it to inherit from SummarizedExperiment but that is not the case, I would suggest using a more descriptive name

As denoted in the above, I am now planning to add more related packages in the future (in a few years?). I would like to let them have similar names. Thus, it would be better for them to have general names not specific ones.

• It doesn't seem to be used by the user, how necessary is it to create a class rather than just using a list?

In future, I plan to GRange, which is included in class but not used now. In this case, class will be better.

I don't think this makes sense and it is confusing for users to use one of the standard Bioconductor object names without inheriting from that object. It is hard to anticipate what kind of object you might need in the future so I would suggest focusing on what you need now. If you are extending the SummarizedExperiment object then this name is ok, if not please use a more specific name to avoid confusion.

• ⚠️ Consider adding checks for whether valid inputs are provided to function arguments

I did, although they might not be proper ones that you intended.

I think these are a good start. This can be difficult so it is usually expanded/improved over time.

• ⚠️ I think some of the for loops could maybe be replaced with other iteration methods, e.g. vapply or lapply

I am not sure. In my ability, I could not. Do you have any specific suggestion for `for' loops?

Some of these could maybe be written using lapply(), for example:

  for (i in seq_len(length(Cancer_cell_lines_p)))
  {
    expDrug <- rbind(expDrug, data.frame(Cancer_cell_lines_p[[i]]))
  }

Could be written something like (untested so maybe need some adjusting):

  for (i in seq_len(length(Cancer_cell_lines_p)))
  {
    expDrug <- rbind(expDrug, data.frame(Cancer_cell_lines_p[[i]]))
  }
  
 expDrug <- lapply(seq_along(Cancer_cell_lines_p), function(i) {
     data.frame(Cancer_cell_lines_p[[i]])
)
expDrug <- do.call("rbind", expDrug)

for loops aren't inherently bad though as long as you pre-allocate memory before which I think you do in most cases. Other cases such as when you are making multiple plots are unavoidable so that's fine. Overall, I don't think you need to worry about this too much.

As a note, you can use seq_along(x) rather than seq_len(length(x)). You can also use 10L rather than as.integer(10)

Shiny apps

• ❓ I thought the plan was to replace the menus with a Shiny app, is this still the case?

I have removed menu and used Shiny app. However, it generated the usage of "<<-" which is noted. Shiny app has not ability to return value, thus I could not avoid the usage of "<<-" to deliver the values gotten in interactive mode to downstream analyses.

I am not very sure if you are satisfied with this revision. If not, I will try to improve it further.

I think this works much better now! I like that you can just specify a value or use the app to select one based on the plots. I have a few minor suggestions. It would maybe be good to add some text to the app describing what to look for in the plots. It might also be easier if the user could see all the plots at once, at the moment I don't think you can go back if you think an earlier one looked better. Can the user look at the plots separately from the selection app? I think that would also be useful, particularly for things like including them in a report.

[tagtag]

Dear @lazappi ,

Unit tests

• 🚨 Please add unit tests for all exported functions. There is some code in the tests file currently by no actual expectation checks

I did.

There are still no actual tests here. As you are using the {testthat} package your tests should be wrapped in the test_that() function. Please refer to the {testthat} documentation or the testing chapter of the R packages book.

I am not sure what you mean. I wrote in testthat.R as

library(testthat)
library(TDbasedUFEadv)

test_check("TDbasedUFEadv")

and in testthat directory I placed

test-TDbasedUFadv.R

Thus test-TDbasedUFadv.R should be executed. This structure is similar to others, for example,

https://code.bioconductor.org/browse/AlpsNMR/tree/master/tests/

I am glad if you can advise me about what the problem is.

Yours, tag.

[bioc-issue-bot]

Received a valid push on git.bioconductor.org; starting a build for commit id: 5bd8090ee9463ec55be0e796778f4028b2d2c39e

[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 build report for more details. This link will be active
for 21 days.

Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/TDbasedUFEadv to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

[tagtag]

Dear @lazappi ,

Thanks for your quick and detailed comments. In the below I tried to address your concerns.

• 🚨 Please use selective imports using importFrom instead of importing all with import

I did.

There are still quite a few normal imports. It is ok if these are unavoidable but I don't think this is the case here. I think some of them ({BiocStyle}, {RTCGA},...) could be made suggested dependencies as they are only used for the vignette/examples, not the core package functionality.

I did. RTCGA cannot be moved to suggested, since a function convertTCGA was used in some functions.

The NEWS file

• ⚠️ Please keep the NEWS file up to date with the latest changes

I did.

I can't see any changes to the NEWS.md file. I just see:

# TDbasedUFEadv 0.99.9

* Addressing many comments from reviewers. 

I modified it. I hope that it is OK now.

Package data

• 🚨 The data in inst/extdata/ should be documented in inst/script

I did.

It's great you added a link to where to get the data but this should also include some information about what the data is, file formats, information it contains, what it is used for etc.

I have added more information.

• ⚠️ It was not clear to me which order to look at the vignettes. I expected to start with TDbasedUFEadv.Rmd but that only convered technical detail, not any information about how to use the package. I suggest renaming/reorganising the vignettes to make this clearer.

It is renamed as "Explanation_of_TDbasedUFEadv"

I think this is better now, it is clear that one vignette explains the method and the other one explains how to use the package. Just double check you have updated the names everywhere, I think I still saw some references to "Quickstart"

I am not sure which one you mean, but a mention about "Quickstart" is not for this package, but the previous one, TDbasedUFE.

• 🟢 Usually it is better to use library(package) rather than require(package). I also find it easier to follow the code if the library calls are in a separate chunk at the start.

I did.

The examples still use require().

I am so sorry. Now all require()s are cheged to library().

Unit tests

• 🚨 Please add unit tests for all exported functions. There is some code in the tests file currently by no actual expectation checks

I did.

There are still no actual tests here. As you are using the {testthat} package your tests should be wrapped in the test_that() function. Please refer to the {testthat} documentation or the testing chapter of the R packages book.

I have commented here.

• ⚠️ I found the SummarizedExperimentTensorRect class a bit unclear in a few ways

  • Given the name I expected it to inherit from SummarizedExperiment but that is not the case, I would suggest using a more descriptive name

As denoted in the above, I am now planning to add more related packages in the future (in a few years?). I would like to let them have similar names. Thus, it would be better for them to have general names not specific ones.

• It doesn't seem to be used by the user, how necessary is it to create a class rather than just using a list?

In future, I plan to GRange, which is included in class but not used now. In this case, class will be better.

I don't think this makes sense and it is confusing for users to use one of the standard Bioconductor object names without inheriting from that object. It is hard to anticipate what kind of object you might need in the future so I would suggest focusing on what you need now. If you are extending the SummarizedExperiment object then this name is ok, if not please use a more specific name to avoid confusion.

I renamed it to TensorRect.

• ⚠️ Consider adding checks for whether valid inputs are provided to function arguments

I did, although they might not be proper ones that you intended.

I think these are a good start. This can be difficult so it is usually expanded/improved over time.

I see.

• ⚠️ I think some of the for loops could maybe be replaced with other iteration methods, e.g. vapply or lapply

I am not sure. In my ability, I could not. Do you have any specific suggestion for `for' loops?

Some of these could maybe be written using lapply(), for example:

  for (i in seq_len(length(Cancer_cell_lines_p)))
  {
    expDrug <- rbind(expDrug, data.frame(Cancer_cell_lines_p[[i]]))
  }

Could be written something like (untested so maybe need some adjusting):

  for (i in seq_len(length(Cancer_cell_lines_p)))
  {
    expDrug <- rbind(expDrug, data.frame(Cancer_cell_lines_p[[i]]))
  }
  
 expDrug <- lapply(seq_along(Cancer_cell_lines_p), function(i) {
     data.frame(Cancer_cell_lines_p[[i]])
)
expDrug <- do.call("rbind", expDrug)

for loops aren't inherently bad though as long as you pre-allocate memory before which I think you do in most cases. Other cases such as when you are making multiple plots are unavoidable so that's fine. Overall, I don't think you need to worry about this too much.

I have changed this one. I could not find any others that can be removed.

As a note, you can use seq_along(x) rather than seq_len(length(x)). You can also use 10L rather than as.integer(10)

I did.

Shiny apps

• ❓ I thought the plan was to replace the menus with a Shiny app, is this still the case?

I have removed menu and used Shiny app. However, it generated the usage of "<<-" which is noted. Shiny app has not ability to return value, thus I could not avoid the usage of "<<-" to deliver the values gotten in interactive mode to downstream analyses.
I am not very sure if you are satisfied with this revision. If not, I will try to improve it further.

I think this works much better now! I like that you can just specify a value or use the app to select one based on the plots. I have a few minor suggestions. It would maybe be good to add some text to the app describing what to look for in the plots. It might also be easier if the user could see all the plots at once, at the moment I don't think you can go back if you think an earlier one looked better. Can the user look at the plots separately from the selection app? I think that would also be useful, particularly for things like including them in a report.

Although I added one sentence, it does not make sense much, since what should be selected is context dependent. This is the reason why I need an interactive mode.
Showing all figures at once is not a good idea, since the number of figure vary. And this process, showing figures one by one, was proposed by some experimentalist who will use this package. He hates to see all figures at once, since it is simply annoying. Actually, it is very likely that selection takes place within the first a few figures.
Figures can be shown, since now batch mode is not dummy but actually generate figures because of your suggestions. Thus users can do as
pdf(file="fig.pdf")
(execute batch mode)
dev.off()
and they can include figures into report.

I am not sure if you are satisfied with the present version yet.

[lazappi]

The NEWS file

• ⚠️ Please keep the NEWS file up to date with the latest changes

I did.

I can't see any changes to the NEWS.md file. I just see:

# TDbasedUFEadv 0.99.9

* Addressing many comments from reviewers. 

I modified it. I hope that it is OK now.

Yes, this is good, thanks! Please keep this up to date as you make changes in the future.

• ⚠️ It was not clear to me which order to look at the vignettes. I expected to start with TDbasedUFEadv.Rmd but that only convered technical detail, not any information about how to use the package. I suggest renaming/reorganising the vignettes to make this clearer.

It is renamed as "Explanation_of_TDbasedUFEadv"

I think this is better now, it is clear that one vignette explains the method and the other one explains how to use the package. Just double check you have updated the names everywhere, I think I still saw some references to "Quickstart"

I am not sure which one you mean, but a mention about "Quickstart" is not for this package, but the previous one, TDbasedUFE.

In Enrichement.Rmd Line 38. If this refers to another package you might want to add a HTML link to the vignette.

Unit tests

• 🚨 Please add unit tests for all exported functions. There is some code in the tests file currently by no actual expectation checks

I did.

There are still no actual tests here. As you are using the {testthat} package your tests should be wrapped in the test_that() function. Please refer to the {testthat} documentation or the testing chapter of the R packages book.

I have commented here.

Yes, I saw that comment and replied to it. As I said before you need to use the test_that() function to turn the code you have into actual tests. What you have now will be run but it is not a test recognised by the build/checking systems. Please check the documentation I linked to above.

This is the one major point left. If you fix this I think I should be able to accept the package.

As a note, you can use seq_along(x) rather than seq_len(length(x)). You can also use 10L rather than as.integer(10)

I did.

There are still a few as.integer() calls that can be replaced in the same way.

Shiny apps

• ❓ I thought the plan was to replace the menus with a Shiny app, is this still the case?

I have removed menu and used Shiny app. However, it generated the usage of "<<-" which is noted. Shiny app has not ability to return value, thus I could not avoid the usage of "<<-" to deliver the values gotten in interactive mode to downstream analyses.
I am not very sure if you are satisfied with this revision. If not, I will try to improve it further.

I think this works much better now! I like that you can just specify a value or use the app to select one based on the plots. I have a few minor suggestions. It would maybe be good to add some text to the app describing what to look for in the plots. It might also be easier if the user could see all the plots at once, at the moment I don't think you can go back if you think an earlier one looked better. Can the user look at the plots separately from the selection app? I think that would also be useful, particularly for things like including them in a report.

Although I added one sentence, it does not make sense much, since what should be selected is context dependent. This is the reason why I need an interactive mode. Showing all figures at once is not a good idea, since the number of figure vary. And this process, showing figures one by one, was proposed by some experimentalist who will use this package. He hates to see all figures at once, since it is simply annoying. Actually, it is very likely that selection takes place within the first a few figures. Figures can be shown, since now batch mode is not dummy but actually generate figures because of your suggestions. Thus users can do as pdf(file="fig.pdf") (execute batch mode) dev.off() and they can include figures into report.

I am not sure if you are satisfied with the present version yet.

I think what you have now is ok, so I am happy with how it is. I do think it could be improved in a few ways though but you can look at that after the package is accepted.

[tagtag]

Dear @lazappi

Thanks for your comments, but I still cannot understand the problem in test.

Yes, I saw that comment and replied to it. As I said before you need to use the test_that() function to turn the code you have into actual tests. What you have now will be run but it is not a test recognised by the build/checking systems. Please check the documentation I linked to above.

This is the one major point left. If you fix this I think I should be able to accept the package.

I compare mine with AlpsNMR, for example.

In AlpsNMR, tests directory is as

and TDbasedUFadv is

Thus they are the same.

For testthat.R AlpsNMR

library(testthat)
library(AlpsNMR)

test_check("AlpsNMR")

for TDbasedUFEadv

library(testthat)
library(TDbasedUFEadv)

test_check("TDbasedUFEadv")

This the same again.

In testthat directory, for AlpsNMR

for TDbasedUFEadv

The only difference is that I have only one file and AlpsNMR has multiple file. What you think the problem is that I have only one file that does not corresponds to individual files in R directory?

If so, there is a reason why I could not prepare the files each of which corresponds to individual files in R directory.
Most of functions in R directory cannot be tested independently, In order to executing A.R requires execute B.R before executing A.R. Thus no test file that test only A.R. Thus we placed one file including all functions in R.

Please advice me how to solve this difficulty if you mean that I have to have multiple files each of which corresponds to individual files in R directory. If you mean something different please let me know. Although I read the document you listed carefully, I cannot understand the problem at all.

Please advise me what to do more practically.

[tagtag]

@lazappi I understand the problem for test. You mean that I have to use test_that{} function in test-TDbasedUFEadv, I think.

[bioc-issue-bot]

Received a valid push on git.bioconductor.org; starting a build for commit id: 765e042bd39a5b767b14125cc0e57e38f603d5c7

[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 build report for more details. This link will be active
for 21 days.

Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/TDbasedUFEadv to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

[tagtag]

Dear @lazappi ,

Thank you very much for you comments. In the bellow, I tied to address your comments,

In Enrichement.Rmd Line 38. If this refers to another package you might want to add a HTML link to the vignette.

Done.

Unit tests

• 🚨 Please add unit tests for all exported functions. There is some code in the tests file currently by no actual expectation checks

I did.

There are still no actual tests here. As you are using the {testthat} package your tests should be wrapped in the test_that() function. Please refer to the {testthat} documentation or the testing chapter of the R packages book.

I have commented here.

Yes, I saw that comment and replied to it. As I said before you need to use the test_that() function to turn the code you have into actual tests. What you have now will be run but it is not a test recognised by the build/checking systems. Please check the documentation I linked to above.

This is the one major point left. If you fix this I think I should be able to accept the package.

I have used test_this() in test-TDbasedUFEadv.
There is only one file, but it was accepted in the previous submission.

There are still a few as.integer() calls that can be replaced in the same way.

Done.

I am not very sure if you are satisfied with this revision. If not, I will try to improve it further.

[lazappi]

Dear @lazappi

Thanks for your comments, but I still cannot understand the problem in test.

Yes, I saw that comment and replied to it. As I said before you need to use the test_that() function to turn the code you have into actual tests. What you have now will be run but it is not a test recognised by the build/checking systems. Please check the documentation I linked to above.
This is the one major point left. If you fix this I think I should be able to accept the package.

I compare mine with AlpsNMR, for example.

In AlpsNMR, tests directory is as and TDbasedUFadv is Thus they are the same.

For testthat.R AlpsNMR

library(testthat) library(AlpsNMR)

test_check("AlpsNMR")

for TDbasedUFEadv

library(testthat) library(TDbasedUFEadv)

test_check("TDbasedUFEadv")

This the same again.

In testthat directory, for AlpsNMR for TDbasedUFEadv

The only difference is that I have only one file and AlpsNMR has multiple file. What you think the problem is that I have only one file that does not corresponds to individual files in R directory?

If so, there is a reason why I could not prepare the files each of which corresponds to individual files in R directory. Most of functions in R directory cannot be tested independently, In order to executing A.R requires execute B.R before executing A.R. Thus no test file that test only A.R. Thus we placed one file including all functions in R.

Please advice me how to solve this difficulty if you mean that I have to have multiple files each of which corresponds to individual files in R directory. If you mean something different please let me know. Although I read the document you listed carefully, I cannot understand the problem at all.

Please advise me what to do more practically.

I can see that you are using the test_that() function now. I'm not sure if you have added that since I last checked or if I missed that earlier (apologies if I did). This is ok but it should be split up into several smaller tests. At the moment if any of these fail you will not be able to tell which check is the issue and where to look in your code. It is not encouraged but it is possible to have code outside the test_that() function. So you would have something more like this:

data(dataset1)

test_that("function 1 works", {
	result <- function1(dataset1)
	expect_indentical(result, expected_result)
})

test_that("function 2 works", {
	output1 <- function1(dataset1)
	result <- function2(output1)
	expect_indentical(result, expected_result)
})

The problem with adding code outside of test_that() is that if there is an error the tests will stop completely whereas if it is inside the function the testing package will handle that. It is ok to have multiple expect_ functions inside a test and this is encouraged to test different parts of the output.

The other important thing is that the name of the test (the first text input to the function) is clear so that it is obvious what the test is checking. This is shown in the output if something fails so it is very helpful for knowing which part of the code to look at.

[bioc-issue-bot]

Received a valid push on git.bioconductor.org; starting a build for commit id: c2d967206da41e9c48ed693528fb443b8e02af43

[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 build report for more details. This link will be active
for 21 days.

Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/TDbasedUFEadv to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

[tagtag]

Dear @lazappi ,

Thank you very much for you prompt replies and comments.

I can see that you are using the test_that() function now. I'm not sure if you have added that since I last checked or if I missed that earlier (apologies if I did). This is ok but it should be split up into several smaller tests. At the moment if any of these fail you will not be able to tell which check is the issue and where to look in your code. It is not encouraged but it is possible to have code outside the test_that() function. So you would have something more like this:

data(dataset1)

test_that("function 1 works", {
	result <- function1(dataset1)
	expect_indentical(result, expected_result)
})

test_that("function 2 works", {
	output1 <- function1(dataset1)
	result <- function2(output1)
	expect_indentical(result, expected_result)
})

The problem with adding code outside of test_that() is that if there is an error the tests will stop completely whereas if it is inside the function the testing package will handle that. It is ok to have multiple expect_ functions inside a test and this is encouraged to test different parts of the output.

The other important thing is that the name of the test (the first text input to the function) is clear so that it is obvious what the test is checking. This is shown in the output if something fails so it is very helpful for knowing which part of the code to look at.

I have tried to fragment test_that() so as to correspond to individual functions, but I was also forced to include many "<<-"s which are advised to avoid as much as possible. However, these functions are tightly interrelated, it was impossible to avoid the usage of "<<-"s.
I am glad if you can accept this version.

Yours, tag.

[lazappi]

I can see that you are using the test_that() function now. I'm not sure if you have added that since I last checked or if I missed that earlier (apologies if I did). This is ok but it should be split up into several smaller tests. At the moment if any of these fail you will not be able to tell which check is the issue and where to look in your code. It is not encouraged but it is possible to have code outside the test_that() function. So you would have something more like this:

data(dataset1)

test_that("function 1 works", {
	result <- function1(dataset1)
	expect_indentical(result, expected_result)
})

test_that("function 2 works", {
	output1 <- function1(dataset1)
	result <- function2(output1)
	expect_indentical(result, expected_result)
})

The problem with adding code outside of test_that() is that if there is an error the tests will stop completely whereas if it is inside the function the testing package will handle that. It is ok to have multiple expect_ functions inside a test and this is encouraged to test different parts of the output.
The other important thing is that the name of the test (the first text input to the function) is clear so that it is obvious what the test is checking. This is shown in the output if something fails so it is very helpful for knowing which part of the code to look at.

I have tried to fragment test_that() so as to correspond to individual functions, but I was also forced to include many "<<-"s which are advised to avoid as much as possible. However, these functions are tightly interrelated, it was impossible to avoid the usage of "<<-"s. I am glad if you can accept this version.

Thanks, this is much better! Each test now checks a single thing and it's easy to see what that is. I think you should be able to avoid using <<-. Some of these aren't necessary because the assigned either isn't used again or is overwritten before it is used. Others could be replaced by either randomly generated data or by pre-computing values and saving them as part of the package. This would also help make the tests independent of each other.

My preference would be for this to be addressed but I don't think it is strictly necessary for the package to be accepted so please let me know what you plan to do.

[bioc-issue-bot]

Received a valid push on git.bioconductor.org; starting a build for commit id: 5efde980d7cc683f3a925614fc9e3e67c27da7ff

[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 build report for more details. This link will be active
for 21 days.

Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/TDbasedUFEadv to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

[tagtag]

Dear @lazappi,

Thank you very much for you quick reply.

I have tried to fragment test_that() so as to correspond to individual functions, but I was also forced to include many "<<-"s which are advised to avoid as much as possible. However, these functions are tightly interrelated, it was impossible to avoid the usage of "<<-"s. I am glad if you can accept this version.

Thanks, this is much better! Each test now checks a single thing and it's easy to see what that is. I think you should be able to avoid using <<-. Some of these aren't necessary because the assigned either isn't used again or is overwritten before it is used. Others could be replaced by either randomly generated data or by pre-computing values and saving them as part of the package. This would also help make the tests independent of each other.

My preference would be for this to be addressed but I don't think it is strictly necessary for the package to be accepted so please let me know what you plan to do.

I have checked one by one the usage of "<<-"s in test because of your advises and erased some of them. I don't think that the remaining (see below) can be either removed or replaced with random numbers since they are class objects that include more than simple numbers.

L10 expDrug -> used later in L14
L20 Z -> used later in L34
L49 SVD -> L61 -> used later in L66
L56 Z -> used later in L65
L80 Z -> used later in L84
L84, L96 Z -> used later in L108

Saving them as pre-computing values is not a good idea either, since they are often huge and might be beyond the individual files size limit of Bioconductor package (5MB).
Thus, we cannot removed more "<<-"s, I think.

I am glad if you can accept the present version.

Yours, tag.

[lazappi]

Dear @lazappi,

Thank you very much for you quick reply.

I have tried to fragment test_that() so as to correspond to individual functions, but I was also forced to include many "<<-"s which are advised to avoid as much as possible. However, these functions are tightly interrelated, it was impossible to avoid the usage of "<<-"s. I am glad if you can accept this version.

Thanks, this is much better! Each test now checks a single thing and it's easy to see what that is. I think you should be able to avoid using <<-. Some of these aren't necessary because the assigned either isn't used again or is overwritten before it is used. Others could be replaced by either randomly generated data or by pre-computing values and saving them as part of the package. This would also help make the tests independent of each other.
My preference would be for this to be addressed but I don't think it is strictly necessary for the package to be accepted so please let me know what you plan to do.

I have checked one by one the usage of "<<-"s in test because of your advises and erased some of them. I don't think that the remaining (see below) can be either removed or replaced with random numbers since they are class objects that include more than simple numbers.

L10 expDrug -> used later in L14 L20 Z -> used later in L34 L49 SVD -> L61 -> used later in L66 L56 Z -> used later in L65 L80 Z -> used later in L84 L84, L96 Z -> used later in L108

Saving them as pre-computing values is not a good idea either, since they are often huge and might be beyond the individual files size limit of Bioconductor package (5MB). Thus, we cannot removed more "<<-"s, I think.

I am glad if you can accept the present version.

Yours, tag.

I will approve the package because there is some testing and I think you have made a big effort to address all my comments. I think you are likely to run into issues with this approach though so I encourage you to look into alternatives.

[bioc-issue-bot]

Your package has been accepted. It will be added to the
Bioconductor nightly builds.

Thank you for contributing to Bioconductor!

Reviewers for Bioconductor packages are volunteers from the Bioconductor
community. If you are interested in becoming a Bioconductor package
reviewer, please see Reviewers Expectations.

[lazappi]

Congratulations, {TDbasedUFEadv} has been accepted into Bioconductor 🎉! It can take a few days to be picked up by the build system but then it should be available in Bioconductor devel and the next Bioconductor release.

[tagtag]

Dear @lazappi ,

Thank you very much for your great efforts to improve my package. I will try to improve this further as much as possible.

Yours, tag.

[lshep]

The default branch of your GitHub repository has been added to Bioconductor's
git repository as branch devel.

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/tagtag.keys is not empty), then no further steps are required. Otherwise, do the following:

1. Add an SSH key to your github account
2. Submit your SSH key to Bioconductor

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/
https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using BiocManager::install("TDbasedUFEadv"). The package 'landing page' will be created at

https://bioconductor.org/packages/TDbasedUFEadv

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.

[tagtag]

@lshep Thank you very much for your very quick job in spite of your very busy schedule during release period!

[lshep]

Thank you for your contributions to Bioconductor!