iSEEindex - Githubissues

kevinrue commented 1 year ago

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

Repository: https://github.com/iSEE/iSEEindex

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.

bioc-issue-bot commented 1 year ago

Hi @kevinrue

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: iSEEindex
Title: iSEE extension for a landing page to a custom collection of data sets
Version: 0.99.0
Date: 2022-12-04
Authors@R: 
    c(person("Kevin", "Rue-Albrecht", email = "kevinrue67@gmail.com",
   role = c("aut", "cre"),
   comment = c(ORCID = "0000-0003-3899-3872")),
    person("Thomas", "Sandmann", email = "tomsing1@gmail.com",
   role = c("ctb"),
   comment = c(ORCID = "0000-0002-6601-8890")),
    person("Denali Therapeutics", role = c("fnd")))
Description: This package provides an interface to any collection of data sets
    within a single iSEE web-application. The main functionality of this package is
    to define a custom landing page allowing app maintainers to list a custom
    collection of data sets that users can selected from and directly load
    objects into an iSEE web-application.
License: Artistic-2.0
URL: https://github.com/iSEE/iSEEindex
BugReports: https://support.bioconductor.org/t/iSEEindex
biocViews: Software,
    Infrastructure
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.3
Depends:
    SummarizedExperiment,
    SingleCellExperiment
Imports:
    BiocFileCache,
    DT,
    iSEE,
    methods,
    paws.storage,
    rintrojs,
    shiny,
    shinydashboard,
    shinyjs,
    stringr,
    urltools,
    utils
Suggests: 
    BiocStyle,
    covr,
    knitr,
    RefManageR,
    rmarkdown,
    sessioninfo,
    testthat (>= 3.0.0),
    yaml
Config/testthat/edition: 3
VignetteBuilder: knitr

kevinrue commented 1 year ago

Tagging @csoneson @federicomarini @LTLA for notifications.

vjcitn commented 1 year ago

A few quick comments. 1) when you have embedded HTML links, set target="_blank" so that the current tab is not lost. The zenodo.org link is an example under zenodo configuration 2) can you propagate more metadata to the index -- particularly size of download.

vjcitn commented 1 year ago

I think the stop app button is viable. Use observe in the server

 observeEvent(input$stopBtn, {
       stopApp(returnValue=NULL)   # could return information here
      })
}

in ui

actionButton("stopBtn", "stop app")

in iSEE place button up near download button ...

kevinrue commented 1 year ago

Fair suggestions. I'll open them as issues, to track them separately from this thread. Especially 1) if they're not no-go for acceptance of the package, 2) the "stop app button" belongs in iSEE, not iSEEindex (as you rightly pointed out yourself).

bioc-issue-bot commented 1 year ago

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 commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

PeteHaitch commented 1 year ago

Hi @kevinrue,

Thank you for submitting iSEEindex to Bioconductor.

I've completed my checklist review of iSEEindex and overall the package is in good shape and close to being ready for acceptance. However, I haven't yet been able to actually try out the app due and this needs to be addressed.

Taking the 'Quick start to using iSEEindex' from the 'Introduction to iSEEindex' vignette, I first tried the ReprocessedAllenData with Default configuration failed with error Graphics API version mismatch. (see screenshot):

I then tried selecting the ReprocessedAllenData with Configuration 1 (R call) but it failed with a different error (see screenshot):

A similar error occurred for the other configurations (replacing iSEEindexRcallResource with iSEEindexHttpsResource for the Zenodo configurations).

Another question I have about the example app is why there is the 'copy' version of the ReprocessedAllenData dataset?

In my checklist review below I have separated the issues into Required and Recommended points that I would ask you to address before the package can be accepted. Would you please provide line-by-line comments to my initial review so that I know what changes I'm looking for in my re-review.

Cheers, Pete

Required

[ ] Does .initial_choices() really need to be exported? It looks like an internal function.

Session info

``` r sessioninfo::session_info() #> ─ Session info ─────────────────────────────────────────────────────────────── #> setting value #> version R version 4.3.0 (2023-04-21) #> os macOS Ventura 13.4 #> system aarch64, darwin20 #> ui X11 #> language (EN) #> collate en_US.UTF-8 #> ctype en_US.UTF-8 #> tz Australia/Melbourne #> date 2023-06-09 #> pandoc 3.1.1 @ /Applications/RStudio.app/Contents/Resources/app/quarto/bin/tools/ (via rmarkdown) #> #> ─ Packages ─────────────────────────────────────────────────────────────────── #> package * version date (UTC) lib source #> Biobase * 2.61.0 2023-04-25 [1] Bioconductor #> BiocFileCache 2.9.0 2023-04-25 [1] Bioconductor #> BiocGenerics * 0.47.0 2023-04-25 [1] Bioconductor #> bit 4.0.5 2022-11-15 [1] CRAN (R 4.3.0) #> bit64 4.0.5 2020-08-30 [1] CRAN (R 4.3.0) #> bitops 1.0-7 2021-04-24 [1] CRAN (R 4.3.0) #> blob 1.2.4 2023-03-17 [1] CRAN (R 4.3.0) #> bslib 0.4.2 2022-12-16 [1] CRAN (R 4.3.0) #> cachem 1.0.8 2023-05-01 [1] CRAN (R 4.3.0) #> circlize 0.4.15 2022-05-10 [1] CRAN (R 4.3.0) #> cli 3.6.1 2023-03-23 [1] CRAN (R 4.3.0) #> clue 0.3-64 2023-01-31 [1] CRAN (R 4.3.0) #> cluster 2.1.4 2022-08-22 [1] CRAN (R 4.3.0) #> codetools 0.2-19 2023-02-01 [1] CRAN (R 4.3.0) #> colorspace 2.1-0 2023-01-23 [1] CRAN (R 4.3.0) #> colourpicker 1.2.0 2022-10-28 [1] CRAN (R 4.3.0) #> ComplexHeatmap 2.17.0 2023-04-25 [1] Bioconductor #> crayon 1.5.2 2022-09-29 [1] CRAN (R 4.3.0) #> curl 5.0.1 2023-06-07 [1] CRAN (R 4.3.0) #> DBI 1.1.3 2022-06-18 [1] CRAN (R 4.3.0) #> dbplyr 2.3.2 2023-03-21 [1] CRAN (R 4.3.0) #> DelayedArray 0.27.4 2023-05-23 [1] Bioconductor #> digest 0.6.31 2022-12-11 [1] CRAN (R 4.3.0) #> doParallel 1.0.17 2022-02-07 [1] CRAN (R 4.3.0) #> dplyr 1.1.2 2023-04-20 [1] CRAN (R 4.3.0) #> DT 0.28 2023-05-18 [1] CRAN (R 4.3.0) #> ellipsis 0.3.2 2021-04-29 [1] CRAN (R 4.3.0) #> evaluate 0.21 2023-05-05 [1] CRAN (R 4.3.0) #> fansi 1.0.4 2023-01-22 [1] CRAN (R 4.3.0) #> fastmap 1.1.1 2023-02-24 [1] CRAN (R 4.3.0) #> filelock 1.0.2 2018-10-05 [1] CRAN (R 4.3.0) #> foreach 1.5.2 2022-02-02 [1] CRAN (R 4.3.0) #> fs 1.6.2 2023-04-25 [1] CRAN (R 4.3.0) #> generics 0.1.3 2022-07-05 [1] CRAN (R 4.3.0) #> GenomeInfoDb * 1.37.1 2023-05-04 [1] Bioconductor #> GenomeInfoDbData 1.2.10 2023-03-26 [1] Bioconductor #> GenomicRanges * 1.53.1 2023-05-04 [1] Bioconductor #> GetoptLong 1.0.5 2020-12-15 [1] CRAN (R 4.3.0) #> ggplot2 3.4.2 2023-04-03 [1] CRAN (R 4.3.0) #> ggrepel 0.9.3 2023-02-03 [1] CRAN (R 4.3.0) #> GlobalOptions 0.1.2 2020-06-10 [1] CRAN (R 4.3.0) #> glue 1.6.2 2022-02-24 [1] CRAN (R 4.3.0) #> gtable 0.3.3 2023-03-21 [1] CRAN (R 4.3.0) #> htmltools 0.5.5 2023-03-23 [1] CRAN (R 4.3.0) #> htmlwidgets 1.6.2 2023-03-17 [1] CRAN (R 4.3.0) #> httpuv 1.6.11 2023-05-11 [1] CRAN (R 4.3.0) #> httr 1.4.6 2023-05-08 [1] CRAN (R 4.3.0) #> igraph 1.4.3 2023-05-22 [1] CRAN (R 4.3.0) #> IRanges * 2.35.1 2023-05-03 [1] Bioconductor #> iSEE 2.13.2 2023-05-19 [1] Bioconductor #> iSEEindex * 0.99.2 2023-06-09 [1] Bioconductor #> iterators 1.0.14 2022-02-05 [1] CRAN (R 4.3.0) #> jquerylib 0.1.4 2021-04-26 [1] CRAN (R 4.3.0) #> jsonlite 1.8.5 2023-06-05 [1] CRAN (R 4.3.0) #> knitr 1.43 2023-05-25 [1] CRAN (R 4.3.0) #> later 1.3.1 2023-05-02 [1] CRAN (R 4.3.0) #> lattice 0.21-8 2023-04-05 [1] CRAN (R 4.3.0) #> lifecycle 1.0.3 2022-10-07 [1] CRAN (R 4.3.0) #> magrittr 2.0.3 2022-03-30 [1] CRAN (R 4.3.0) #> Matrix 1.5-4 2023-04-04 [1] CRAN (R 4.3.0) #> MatrixGenerics * 1.13.0 2023-04-25 [1] Bioconductor #> matrixStats * 1.0.0 2023-06-02 [1] CRAN (R 4.3.0) #> memoise 2.0.1 2021-11-26 [1] CRAN (R 4.3.0) #> mgcv 1.8-42 2023-03-02 [1] CRAN (R 4.3.0) #> mime 0.12 2021-09-28 [1] CRAN (R 4.3.0) #> miniUI 0.1.1.1 2018-05-18 [1] CRAN (R 4.3.0) #> munsell 0.5.0 2018-06-12 [1] CRAN (R 4.3.0) #> nlme 3.1-162 2023-01-31 [1] CRAN (R 4.3.0) #> paws.common 0.5.6 2023-03-06 [1] CRAN (R 4.3.0) #> paws.storage 0.2.0 2023-02-08 [1] CRAN (R 4.3.0) #> pillar 1.9.0 2023-03-22 [1] CRAN (R 4.3.0) #> pkgconfig 2.0.3 2019-09-22 [1] CRAN (R 4.3.0) #> png 0.1-8 2022-11-29 [1] CRAN (R 4.3.0) #> promises 1.2.0.1 2021-02-11 [1] CRAN (R 4.3.0) #> R6 2.5.1 2021-08-19 [1] CRAN (R 4.3.0) #> RColorBrewer 1.1-3 2022-04-03 [1] CRAN (R 4.3.0) #> Rcpp 1.0.10 2023-01-22 [1] CRAN (R 4.3.0) #> RCurl 1.98-1.12 2023-03-27 [1] CRAN (R 4.3.0) #> reprex 2.0.2 2022-08-17 [1] CRAN (R 4.3.0) #> rintrojs 0.3.2 2022-08-09 [1] CRAN (R 4.3.0) #> rjson 0.2.21 2022-01-09 [1] CRAN (R 4.3.0) #> rlang 1.1.1 2023-04-28 [1] CRAN (R 4.3.0) #> rmarkdown 2.22 2023-06-01 [1] CRAN (R 4.3.0) #> RSQLite 2.3.1 2023-04-03 [1] CRAN (R 4.3.0) #> rstudioapi 0.14 2022-08-22 [1] CRAN (R 4.3.0) #> S4Arrays 1.1.4 2023-05-14 [1] Bioconductor #> S4Vectors * 0.39.1 2023-05-03 [1] Bioconductor #> sass 0.4.6 2023-05-03 [1] CRAN (R 4.3.0) #> scales 1.2.1 2022-08-20 [1] CRAN (R 4.3.0) #> sessioninfo 1.2.2 2021-12-06 [1] CRAN (R 4.3.0) #> shape 1.4.6 2021-05-19 [1] CRAN (R 4.3.0) #> shiny 1.7.4 2022-12-15 [1] CRAN (R 4.3.0) #> shinyAce 0.4.2 2022-05-06 [1] CRAN (R 4.3.0) #> shinydashboard 0.7.2 2021-09-30 [1] CRAN (R 4.3.0) #> shinyjs 2.1.0 2021-12-23 [1] CRAN (R 4.3.0) #> shinyWidgets 0.7.6 2023-01-08 [1] CRAN (R 4.3.0) #> SingleCellExperiment * 1.23.0 2023-04-25 [1] Bioconductor #> SparseArray 1.1.9 2023-05-29 [1] Bioconductor #> stringi 1.7.12 2023-01-11 [1] CRAN (R 4.3.0) #> stringr 1.5.0 2022-12-02 [1] CRAN (R 4.3.0) #> SummarizedExperiment * 1.31.1 2023-05-01 [1] Bioconductor #> tibble 3.2.1 2023-03-20 [1] CRAN (R 4.3.0) #> tidyselect 1.2.0 2022-10-10 [1] CRAN (R 4.3.0) #> triebeard 0.4.1 2023-03-04 [1] CRAN (R 4.3.0) #> urltools 1.7.3 2019-04-14 [1] CRAN (R 4.3.0) #> utf8 1.2.3 2023-01-31 [1] CRAN (R 4.3.0) #> vctrs 0.6.2 2023-04-19 [1] CRAN (R 4.3.0) #> vipor 0.4.5 2017-03-22 [1] CRAN (R 4.3.0) #> viridisLite 0.4.2 2023-05-02 [1] CRAN (R 4.3.0) #> withr 2.5.0 2022-03-03 [1] CRAN (R 4.3.0) #> xfun 0.39 2023-04-20 [1] CRAN (R 4.3.0) #> xtable 1.8-4 2019-04-21 [1] CRAN (R 4.3.0) #> XVector 0.41.1 2023-05-03 [1] Bioconductor #> yaml 2.3.7 2023-01-23 [1] CRAN (R 4.3.0) #> zlibbioc 1.47.0 2023-04-25 [1] Bioconductor #> #> [1] /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library #> #> ────────────────────────────────────────────────────────────────────────────── ```

kevinrue commented 1 year ago

Thanks Pete!

All fair comments.

I'm genuinely surprised about some of those issues, as I don't remember ever running into them myself, and I've already heard from some independent early adopters (who installed the package from GitHub) who successfully used the app, but I'm very keen to investigate why this is happening to you.

I'll keep you posted as soon as I get to it.

Thanks again Kevin

kevinrue commented 1 year ago

@PeteHaitch

Taking the 'Quick start to using iSEEindex' from the 'Introduction to iSEEindex' vignette, I first tried the ReprocessedAllenData with Default configuration failed with error Graphics API version mismatch.

I cannot reproduce the error. See below.

Since I don't have the issue, I cannot test any fix, but Googling led me here: https://stackoverflow.com/questions/68753250/getting-the-error-graphics-api-version-mismatch

In particular, the top answer states:

Can you give that a go please, while I answer to your other points in separate comments?

PeteHaitch commented 1 year ago

We're coming into a long weekend here, I'll take another look after Tuesday.

kevinrue commented 1 year ago

OK, no problem, I'm teaching full time for the next couple of weeks, so I might only sneak in a bit of work on this here and there anyway. Don't mind me if I post a few more replies to you other points as I go through them :)

kevinrue commented 1 year ago

For your second screenshot (error Configuration 1 (R call) and other types of initial configurations), that's entirely on me for not thoroughly testing the app following a recent change in the internal management of initial configurations.

I've identified the issue, and will release a new version that includes the fix hopefully later this week.

Thanks!

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

kevinrue commented 1 year ago

@PeteHaitch

Another question I have about the example app is why there is the 'copy' version of the ReprocessedAllenData dataset?

I'll give you the context below, and I'll appreciate any suggestion for a better / more intuitive name from your (user) perspective.

For demonstration purposes, I've preprocessed and uploaded a data set to zenodo. Now, to demonstrate a choice of data sets in the app, rather than preprocess a second data set, add it to the zenodo record, etc., I've decided to list the same data set under a different name in the app itself, hence 'copy'.

In other words, the two choices in the app point to the same RDS file on zenodo.

Now that it's all working, I'm open to putting more work in that direction if asked, as it isn't really at the top of my priority list unless of course the current setup creates too much confusion.

Options:

1) preprocess a second data set, upload it to zenodo, and update the choice of data sets in the demo app to reflect this 2) keep duplicated data set but label it with a better name / description to clarify 3) other ideas?

bioc-issue-bot commented 1 year ago

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

kevinrue commented 1 year ago

Re: your checklist

Required

[x] Does .initial_choices() really need to be exported? It looks like an internal function.

Well spotted. Looks like an accidental leftover.

Recommended

[x] The formatting of session info in both vignettes is a bit odd compared to other code chunks in the vignette. Is that intentional?
[x] Consider adding a package man page.

(done!)

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

kevinrue commented 1 year ago

@PeteHaitch

The formatting of session info in both vignettes is a bit odd compared to other code chunks in the vignette. Is that intentional?

I'm not entirely sure what you mean here.

Is it the fact that some lines wrap over two lines? Using Leo's biocthis template sets options(width=120), while I see that pkgdown seems to limit the width of code blocks to ~105.

I'm about to change the vignettes to options(width=100) to make it look nicer. I hope that's what you meant.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

kevinrue commented 1 year ago

@PeteHaitch

As a summary:

I addressed all the issues you raised, required and recommended
In this comment I recommend reinstalling Cairo based on online advice, as this look like an issue with your R installation rather than iSEEindex
In this comment I indicate that I've identified the issue, which I have fixed now.
In this this comment, I explain my motivation for making a copy of the test data set. Happy to get feedback and suggestions from you to make it clearer, especially for less experienced users.

Thanks for you review!

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

PeteHaitch commented 1 year ago

Thanks for the updates, @kevinrue.

I've now been able to actually run the app. Re-installing ragg (not Cairo) from source fixed the issue for me ( based on https://stackoverflow.com/a/73210946). For what it's worth, I'm using an Apple silicon (M2) macbook air running R-4.3.0-arm64.

Now that it's all working, I'm open to putting more work in that direction if asked, as it isn't really at the top of my priority list unless of course the current setup creates too much confusion.

Options:

preprocess a second data set, upload it to zenodo, and update the choice of data sets in the demo app to reflect this

keep duplicated data set but label it with a better name / description to clarify

other ideas?

I think 1 is clearly the best option given the point is to demonstrate that multiple datasets could be available via an iSEEindex instance and the user would have a choice of the available dataset. As the app currently (i.e. effectively option 2), I think it's just confusing and distracts from the point you're trying to make. Another idea would be to just have 1 dataset - I think that's better than option (2) but worse than (1). I'd strongly recommend option (1).

The formatting of session info in both vignettes is a bit odd compared to other code chunks in the vignette. Is that intentional?

Sorry, this wasn't the most descriptive or helpful comment. Hopefully this clarifies what I mean. The session info doesn't include the grey background or other formatting used by other R code chunks in the vignette.

That's different to what I'm used to seeing in other Bioconductor packages (e.g., https://bioconductor.org/packages/release/bioc/vignettes/iSEE/inst/doc/basic.html#Session_Info).

Cheers, Pete

kevinrue commented 1 year ago

I think 1 is clearly the best option

I'll prepare a second data set and go with option (1).

The session info doesn't include the grey background or other formatting used by other R code chunks in the vignette.

Strange, I see the grey background in my pkgdown:

Although I do see the same surprising lack of grey background when using browseVignettes.

The code and options of the sessioninfo chunk is similar to other chunks above, which display the grey background perfectly fine.

However, somehow, the web inspector indicates that the previous "good" block has HTML as follows:

<div>
  <pre class="r">
    <code class="hljs">

while the sessioninfo misses the div and the class="r"

<pre>
    <code class="hljs">

I can dig a bit further but I've checked and it doesn't even look like a difference between sessionInfo() and sessioninfo::session_info(); they're both afffected in the same way at this point.

I've been using biocthis templates for this package while iSEE did not use those templates. I'll have to bring @lcolladotor in at some point, either to help me figure out the issue and/or to update the templates to fix that glitch.

EDIT the same vignette template seems to be working fine for biocthis on the bioc website, see https://www.bioconductor.org/packages/release/bioc/vignettes/biocthis/inst/doc/biocthis.html#5_Reproducibility Perhaps we can ignore that glitch during this review process, and see if it solves itself on the main build system?

I'll still work on the second demonstration data set for now, which seems to be your last request before approval.

LTLA commented 1 year ago

Maybe a pandoc issue, see Bioconductor/BiocStyle#103.

lcolladotor commented 1 year ago

Aye, this issue doesn't seem related to biocthis. What Pete is looking at is the output of rendering the vignette with BiocStyle::html_document(), which as Aaron has pointed out, is the one that interacts with pandoc.

For pkgdown, if you use https://lcolladotor.github.io/biocthis/reference/use_bioc_pkgdown_css.html then it does add a pkgdown css config file, which @kevinrue wrote and knows more about it than me. My understanding is that that css file (aka https://github.com/lcolladotor/biocthis/blob/devel/inst/templates/bioc-pkgdown-extra.css) is mostly changing colors. Not shapes of boxes.

PeteHaitch commented 1 year ago

Perhaps we can ignore that glitch during this review process, and see if it solves itself on the main build system?

That's fine. It's not a requirement to fix this, but I wanted to draw it to your attention because I thought it may be something you may want to change/fix.

PeteHaitch commented 1 year ago

I'll check the version of pandoc I used to build the vignette locally (and where my screenshot came from) but it does look similar to the issue Aaron reported and linked to

PeteHaitch commented 1 year ago

> rmarkdown::pandoc_version()
[1] ‘3.1.1’

Consistent with https://github.com/Bioconductor/BiocStyle/issues/103

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year ago

Dear Package contributor,

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

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

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

Please see the build report for more details. 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/iSEEindex to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

kevinrue commented 1 year ago

I'll prepare a second data set and go with option (1).

That's all done now. I've replaced the copy of the first data set with a second (distinct) data set that comes with its own different initial configuration.

Anything else, let me know!

PeteHaitch commented 1 year ago

Will take a look this week and expect I'll then be able to quickly accept the package.

PeteHaitch commented 1 year ago

Thank you for making the requested changes, @kevinrue . I'm happy to accept iSEEindex into Bioconductor. Congratulations and thank you for your contribution!

A reminder of one question I had from an earlier comment where I'd appreciate an answer:

[ ] Is refreshing the webpage or relaunching the app the only way to return to the launch page, e.g., to select a different dataset? Could you add a button to do this because it wasn't obvious to me how to return to the launch page.

And a few minor things for you to tidy up:

[ ] Remember to update the screenshots in the vignette, e.g., it still shows the "Reprocessed Allen Data (copy)" dataset in the introduction vignette.
[ ] Please check the metadata of 'BuettnerESCData'; I see "Reprocessed Allen Data (copy)" when I launch the app following the code in the introduction vignette (see screenshot).

bioc-issue-bot commented 1 year ago

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.

Bioconductor / Contributions

iSEEindex #3064

Required

Recommended

Session info