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]'

• [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 Bioconductor Package Naming Policy and acknowledge Bioconductor may retain use of package name.

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

• [x] 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:

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

[tagtag]

@LiNk-NY

I had not realised the batch mode is not functional and only there to pass the tests. This is something I think we want to avoid. It could be very confusing to the user and means the functions are not tested properly. I am beginning to think that Shiny is the best option in this case. That would give a nicer interface to the user than the menus and would make it easier to separate the processing code and the interface so that they can be checked properly.

What do you mean? Even if we are using Shiny, it is impossible to include figures generated from interactive mode, we have to put figures generated from Shiny outside vignettes. Am I wrong? Or if I use Shiny, figures generated from Shiny can be included in vignettes automatically? I am very confused. What is improved if we employ Shiny instead of menu?

[lazappi]

I had not realised the batch mode is not functional and only there to pass the tests. This is something I think we want to avoid. It could be very confusing to the user and means the functions are not tested properly. I am beginning to think that Shiny is the best option in this case. That would give a nicer interface to the user than the menus and would make it easier to separate the processing code and the interface so that they can be checked properly.

What do you mean? Even if we are using Shiny, it is impossible to include figures generated from interactive mode, we have to put figures generated from Shiny outside vignettes. Am I wrong? Or if I use Shiny, figures generated from Shiny can be included in vignettes automatically? I am very confused. What is improved if we employ Shiny instead of menu?

Sorry I wasn't clear, I will try to be more specific. I think:

• Using Shiny would give a nicer interface to the user
• Using Shiny would allow you to better separate the interactive and functional code. It's probably still possible to do that for other approaches though.
• No, using Shiny won't allow you to include plots directly in the vignette but separating the plotting and visualisation code would. You could then show an example of a plot without going through the whole selection process.
• Showing the plots I think is a secondary issue, it would be fine to include screenshots of the app (or whatever interface), but maybe you don't need as many as you have now.
• In the end how to do this is up to you. We are just trying to provide suggestions that we think will lead to a better experience for the user and a more maintainable package for you.

Also, I am the main reviewer for your package. I am sure @LiNk-NY is happy to provide input if needed but please stop pinging them in every message.

[tagtag]

@lazappi

I had not realised the batch mode is not functional and only there to pass the tests. This is something I think we want to avoid. It could be very confusing to the user and means the functions are not tested properly. I am beginning to think that Shiny is the best option in this case. That would give a nicer interface to the user than the menus and would make it easier to separate the processing code and the interface so that they can be checked properly.

What do you mean? Even if we are using Shiny, it is impossible to include figures generated from interactive mode, we have to put figures generated from Shiny outside vignettes. Am I wrong? Or if I use Shiny, figures generated from Shiny can be included in vignettes automatically? I am very confused. What is improved if we employ Shiny instead of menu?

Sorry I wasn't clear, I will try to be more specific. I think:

• Using Shiny would give a nicer interface to the user
• Using Shiny would allow you to better separate the interactive and functional code. It's probably still possible to do that for other approaches though.
• No, using Shiny won't allow you to include plots directly in the vignette but separating the plotting and visualisation code would. You could then show an example of a plot without going through the whole selection process.
• Showing the plots I think is a secondary issue, it would be fine to include screenshots of the app (or whatever interface), but maybe you don't need as many as you have now.
• In the end how to do this is up to you. We are just trying to provide suggestions that we think will lead to a better experience for the user and a more maintainable package for you.

Also, I am the main reviewer for your package. I am sure @LiNk-NY is happy to provide input if needed but please stop pinging them in every message.

Without including interactive mode, I cannot demonstrate downstream analysis, since only after the selections through interactive mode, we can have the input for downstream analysis. The users needs to investigate plot one by one, to perform selection. It is impossible to generate batch mode to provide results without interactive mode. Do you understand the problem? You said that batch mode without doing anything is not allowed. But intearctive mode is not allowed in vignettes. But we need results form inteerctive mode. If dummy bacth mode is not allowed, I cannot generate vignettes at all. Did you agree with including dummy batch mode now? Or are you still opposed to this? In this case, please let me know the acceptable way by which we can get the result from interactive mode without either performing interactive mode or including dummy batch mode that simply provides the result when interactive mode was performed. Did you understand the points? I need the answer on this point.

[bioc-issue-bot]

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

[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". 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: 704e298a919fb864e772998ae987443df361e190

[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". 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 This error seemed to come from the system, not from mine. I am now asking this issue in Slack. https://community-bioc.slack.com/archives/C34NC134G/p1679156164253489

[tagtag]

@lazappi I have stopped employing dummy batch mode and follow your advice. I have found some solution on how to avoid fig.keep="none". I will inform you when I address all the concerns you raised as much as possible.

[bioc-issue-bot]

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

[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". 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 ,

Thanks for your detailed comments. I tried to address them. Although my package shows Error, It is not because of my side but system side (see https://community-bioc.slack.com/archives/C34NC134G/p1679197255196319?thread_ts=1679156164.253489&cid=C34NC134G). As you can see below, it has passed in Mac. Thus, it might be OK for you to comment revised one. If not, I will wait for the system probelm is solved.

Please address the comments as best as you can. When you are ready for me to check the package again please reply to let me know with a summary of changes you have made or any other responses. You can use the "Quote reply" option in the ... menu on this comment to reply directly to my points below.

Luke

Review

Key: 🚨 Required ⚠️ Recommended 🟢 Optional ❓ Question

General comments

• [ ] ❓ The functions in the package seem to mostly be small helper functions that build on the functionality in {TDbasedUFE}. Have you considered adding them to the existing package rather than creating a new one?

Yes, I have considered adding them to the existing package. However, it will be too huge to install. I might add more functions to it. In addition to this, the functions in this package and those in future packaged might not be required by many peoplr. Thus, I think that it mght b better to have this one as a separated one.

DESCRIPTION file

• [ ] ⚠️ It may be appropriate to add "Software" to the biocViews field

I have added.

• [ ] 🟢 It is recommended to add a BugReports field, this is usually a link to the GitHub issues page or the Bioconductor support site

I have added.

• [ ] 🟢 It is recommended to add a URL field, this is usually a link to the GitHub repository

I have added.

NAMESPACE file

• [ ] 🚨 PrepareSummarizedExperimentTensorRect should start with a lower case letter to match the other functions

I did.

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

I did.

The NEWS file

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

I did.

Package data

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

I did.

Documentation

• [ ] 🟢 There were quite a lot of typos in different places, the {spelling} package can help with finding these

I did.

README

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

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

• [ ] ⚠️ It doesn't look as though you are using any RMarkdown functionality, if so it will be simpler to use a plain README.md file instead of README.Rmd

I did.

Vignette

• [ ] 🚨 Please expand the introduction in the main vignette to describe the package in more detail. This should include the motivation for including the package in Bioconductor and comparisions to any similar packages (if applicable).

I did.

• [ ] ⚠️ 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"

• [ ] ⚠️ It also wasn't clear to me what the difference was between Quickstart.Rmd and Quickstart2.Rmd. I would suggest that this can be combined.

They were merged and renamed as "How_to_use_TDbasedUFEadv"

• [ ] 🚨 Whatever you decide to do I suggest using a more specific file name than "QuickStart.Rmd", sometimes it causes issues if different packages have vignettes with the same name

I did as above.

• [ ] 🚨 The main vignette should have an installation section that includes the Bioconductor installation instructions. These chunk should not be evaluated.

I did.

• [ ] ⚠️ There are lots of images in the vignettes directory, are all of these required or can some be produced by the vignette (I think this is related to the point about interactivity below)?

Figs were removed.

• [ ] ⚠️ I don't think you need to show all the menu items, I think it would be enough to mention there is an interactive option and maybe show one or two screenshots

Figs were removed.

• [ ] ⚠️ Please don't use fig.keep="none" if it can be avoided, in general the vignette should show the same output as the user would see if they ran the code themselves

Now most figures generated inside vignettes. Some conceptual figures remain there.

• [ ] 🟢 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.

Man pages

• [ ] 🚨 You have included the word "Title" in the function documentation title which is then rendered in the final help page, either use @title TEXT or just TEXT on the first line

I did.

• [ ] 🟢 Some of the variables in the examples are not very descriptive (e.g. dummy) which makes them a bit hard to understand

I renamed them.

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.

Code

R

• [ ] 🚨 Please use named indicies instead of numeric indices where possible

I did as much as possible, but I found only a few.

• [ ] ⚠️ 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.

• [ ] It is defined in a file called init.R which is not clear, usually this file is called something like AllClasses.R or the name of the class

I did.

• [ ] ⚠️ Please consider using more descriptive argument names, some of them (e.g. "k", "j", "LIST", "L") do not clearly describe what they are used for

I did.

• [ ] ⚠️ 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 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?

• [ ] ⚠️ Generally functions should not output plots as side effects, generally it is better to have separate analysis and plotting functions

Interactive mode is required in this implementation. They must be executed in batch mode in vignettes and must generate figures in vignettes. Please let them to generate figures.

• [ ] ⚠️ It may be better to separate interactive functions for the non-interactive versions (or change how the options work), I found it a bit surprising that interactive was the default mode. Maybe one function could produce a plot that the user uses to select a parameter which they then pass to another function?

Functions in interactive mode must generate figures. Otherwise, no interactive function can work.

• [ ] 🟢 This is mostly personal preference but I found your code quite hard to read because of the lack of indenting and whitespace. You may want to look at the tidyverse style guide or the styler package.

I have reformed them using styler package as suggested.

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.

Thanks for your efforts again.

[bioc-issue-bot]

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

[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". 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: ce43ffdfbd9916cfbdf2429386dbc877978c5497

[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". 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: 4036ac657aedff2db119b0a449b82d6d28dcc945

[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". 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: 74c35487559470df2fffbee03b04033506d95ebd

[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". 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: 5b757d80f595d96c65d3b9c1d4f42d7376328399

[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". 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 ,

Although build system problem was not yet solved, since the present version 0.99.14 passed check in Mac, I am glad if you can review the present version to which I have replied to your comments at issuecomment-1476367945.

Yours, tag.

[bioc-issue-bot]

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

[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 ,

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.