search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: biopixR: Extracting Insights from Biological Images #7074

Closed editorialbot closed 1 month ago

editorialbot commented 3 months ago

Submitting author: !--author-handle-->@Brauckhoff<!--end-author-handle-- (Tim Brauckhoff) Repository: https://github.com/Brauckhoff/biopixR Branch with paper.md (empty if default branch): Version: 1.2.0 Editor: !--editor-->@fabian-s<!--end-editor-- Reviewers: @ColemanRHarris, @tijeco Archive: 10.5281/zenodo.13899162

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/80bda20fb774fd758a14b5ff02aaed68"><img src="https://joss.theoj.org/papers/80bda20fb774fd758a14b5ff02aaed68/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/80bda20fb774fd758a14b5ff02aaed68/status.svg)](https://joss.theoj.org/papers/80bda20fb774fd758a14b5ff02aaed68)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@ColemanRHarris & @tijeco, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @fabian-s know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @tijeco

📝 Checklist for @ColemanRHarris

editorialbot commented 3 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 3 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.06394 is OK
- 10.32614/CRAN.package.biopixR is OK
- 10.21037/jlpm.2019.04.05 is OK
- 10.1016/j.biosx.2024.100484 is OK
- 10.32614/rj-2011-002 is OK
- 10.32614/RJ-2012-018 is OK
- 10.32614/CRAN.package.countcolors is OK
- 10.21105/joss.01012 is OK
- 10.32614/CRAN.package.magick is OK
- 10.1007/978-0-387-75936-4 is OK
- 10.1109/tsmc.1973.4309314 is OK
- 10.18637/jss.v021.i05 is OK
- 10.32614/CRAN.package.cluster is OK
- 10.1007/s00604-019-3449-y is OK
- 10.1080/17425247.2016.1192122 is OK
- 10.1007/s10544-018-0314-4 is OK
- 10.1007/s00604-014-1243-4 is OK
- 10.1038/76469 is OK
- 10.1007/s11356-020-08127-2 is OK
- 10.1038/s41598-019-41136-x is OK
- 10.1021/ac103277s is OK
- 10.3390/molecules201219766 is OK
- 10.1007/s00216-019-02199-x is OK
- 10.1007/10_2011_132 is OK
- 10.21037/jlpm.2018.11.01 is OK
- 10.21037/jlpm.2018.04.10 is OK
- 10.18637/jss.v049.i09 is OK
- 10.1198/106186007X178663 is OK
- 10.32614/RJ-2015-011 is OK
- 10.5281/ZENODO.12744222 is OK

MISSING DOIs

- No DOI given, and none found for title: R: A Language and Environment for Statistical Comp...
- No DOI given, and none found for title: Tcl/Tk Interface
- No DOI given, and none found for title: RStudio: Integrated Development Environment for R
- No DOI given, and none found for title: Advanced R
- No DOI given, and none found for title: ’Radiomic’ Image Processing Toolbox
- No DOI given, and none found for title: R Markdown
- No DOI given, and none found for title: foodwebr: Visualise Function Dependencies

INVALID DOIs

- None
editorialbot commented 3 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.05 s (1134.5 files/s, 257540.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               35            472           1159           2524
TeX                              2            161              0           1868
HTML                             1            338              5           1725
Rmd                              7            624           2084           1241
Markdown                         5            171              0            580
YAML                             2             29             18            128
CSV                              6              0              0             40
-------------------------------------------------------------------------------
SUM:                            58           1795           3266           8106
-------------------------------------------------------------------------------

Commit count by author:

   236  Brauckhoff
    70  devSJR
     2  Michael Chirico
editorialbot commented 3 months ago

Paper file info:

📄 Wordcount for paper.md is 1986

✅ The paper includes a Statement of need section

editorialbot commented 3 months ago

License info:

🟡 License found: GNU Lesser General Public License v3.0 (Check here for OSI approval)

editorialbot commented 3 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

fabian-s commented 3 months ago

hi @tijeco & @ColemanRHarris,

this is the review thread for the paper. All of our communications will happen here from now on.

As a reviewer, the first step is to create a checklist for your review by entering

@editorialbot generate my checklist

as the top of a new comment in this thread.

These checklists contain the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. The first comment in this thread also contains links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#7074 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use EditorialBot (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@fabian-s) if you have any questions/concerns.

tijeco commented 3 months ago

Review checklist for @tijeco

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

fabian-s commented 3 months ago

@Brauckhoff
while we wait for reviewers to start working on this, please start fixing the missing DOIs the bot pointed out above.

devSJR commented 3 months ago

@editorialbot generate pdf

devSJR commented 3 months ago

Dear @fabian-s, thanks for pointing out the issue with the DOIs. We are eager to cite everything we use or refer to with state-of-the-art technologies. Therefore, we fixed most missing DOIs shortly after submission during the PRE REVIEW. It appears, that we missed a DOI for RMarkown, which I just added.

However, we fear, there is little we can do for the others, since these items have no DOI. For example:

Is this acceptable, or do you see another solution to this?

editorialbot commented 3 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

devSJR commented 3 months ago

@editorialbot generate pdf

editorialbot commented 3 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

ColemanRHarris commented 3 months ago

Review checklist for @ColemanRHarris

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

fabian-s commented 3 months ago

However, we fear, there is little we can do for the others, since these items have no DOI. For example:

* [foodwebr](https://github.com/lewinfox/foodwebr) is on GitHub only and

* [radiomics is archived on CRAN](https://cran.r-project.org/web/packages/radiomics/index.html). We cite radiomics because it is an R package, which allows extracting Haralick features and clusters information using Partitioning Around Medoids (PAM). However, we will remove the reference to the package since the reader is still guided to appropriate literature.

Is this acceptable, or do you see another solution to this?

@devSJR thanks, perfect!

devSJR commented 3 months ago

Just a short note. We have added a new function to the package, which didn't end up in a branch but in main. Sorry for that, we will fix that. The package submitted for review is the stable version 1.x after bug fixes (→ 1.1.0).

devSJR commented 3 months ago

Dear @fabian-s, @tijeco and @ColemanRHarris,

we forgot to mention that we already moved our latest commit, which caused a problem:

Just a short note. We have added a new function to the package, which didn't end up in a branch but in main. Sorry for that, we will fix that. The package submitted for review is the stable version 1.x after bug fixes (→ 1.1.0).

For the moment being, we decided to halt all pending commits of experimental new functions and new data sets, since we haven't tested them intensively in our current research. We decided it is also better not to submit anything until the review process is finished. Better than saying, “Let's commit. What could possibly go wrong?”, in my humble opinion. I hope you consent with this approach.

tijeco commented 3 months ago

@fabian-s , for clarity, we should be reviewing https://github.com/Brauckhoff/biopixR/tree/1880095c8bca6701b5363857f1cf4bba9738379b, corresponding to their release 1.1.0?

tijeco commented 3 months ago

@devSJR Could you provide clarification about contribution and authorship? The authors in the manuscript are Tim Brauckhoff, Coline Kieffer, and Stefan Rödiger. The first and last authors appear to have made contributions in the commit history. I don't see the middle author's contribution. Also, it looks like Michael Chirico contributed to the software via pull request, but I don't see them in the manuscript.

devSJR commented 3 months ago

Dear @tijeco, sure, I can do that.

Tim does the main programming and documentation of biopixR.

I lead this project (active project direction) and usually review the code (sometimes we even did paired programming), make contributions, fixes, and help with the documentation.

Coline Kieffer contributes the various datasets that are central to the package. For this, she prepared wet lab samples, did the imaging and subsequent insight into the data jointly with us. So to speak, she provided the oil. She also, raised issues (albeit personally in the lab or by decentralized digital messaging) and reflects part of the target audience (biologists in the wet lab). We also specifically mention her in the supplement to this paper https://zenodo.org/doi/10.5281/zenodo.12744222 on page 78 (this supplement is referenced as Brauckhoff, T., & Rödiger, S. (2024). biopixR: Extracting insights from biological images. https://doi.org/10.5281/ZENODO.12744222). You will certainly notice that there are more people mentioned. However, considering invested time and dedication, Coline certainly contributed significantly.

Michael Chirico contributed, indeed a software pull request. But truth to be told, I don't know if we should/must include him. @fabian-s, would you be so kind and tell me how to deal with this?

I hope this answers your question.

fabian-s commented 3 months ago

@tijeco

for clarity, we should be reviewing https://github.com/Brauckhoff/biopixR/tree/1880095c8bca6701b5363857f1cf4bba9738379b, corresponding to their release 1.1.0?

that's how i understood @devSJR 's remark, too.

fabian-s commented 3 months ago

@devSJR

Michael Chirico contributed, indeed a software pull request. But truth to be told, I don't know if we should/must include him. @fabian-s, would you be so kind and tell me how to deal with this?

I looked at their 2 commits, seems like very small housekeeping stuff to me. I don't think their contribs warrant a co-authorship.

fabian-s commented 2 months ago

@ColemanRHarris please remember to fill out your review checklist soon -- how much longer do you think you will need?

fabian-s commented 2 months ago

@tijeco please remember to complete your review checklist -- how much longer do you think you will need? if you have identified any problems, please let @devSJR & co know by opening up issues in their repo or describing them in this thread.

ColemanRHarris commented 2 months ago

@ColemanRHarris please remember to fill out your review checklist soon -- how much longer do you think you will need?

Hi @fabian-s, thanks for the nudge! I will get to this early next week, so give me 1 more week and I'll be good to go. Thanks!

fabian-s commented 1 month ago

@tijeco ping ;)

@tijeco please remember to complete your review checklist -- how much longer do you think you will need? if you have identified any problems, please let @devSJR & co know by opening up issues in their repo or describing them in this thread.

tijeco commented 1 month ago

@fabian-s and @devSJR Hi! I'm working on wrapping things up. I had some issues getting things to install on my mac without root priveleges. So I did get it working in a github codespace. I noticed the specific dependencies are noted in the manuscript, but they don't seem to be noted in the README, things like the version of R, imager, magick etc. I can raise that as an issue in the repo if you'd like!

devSJR commented 1 month ago

@tijeco thanks for testing!

We have no Macs, but we use CI and CRAN, which gave us no issues. Thus, we missed that.

An issue would be good. These things are fixable timely.

fabian-s commented 1 month ago

@fabian-s and @devSJR Hi! I'm working on wrapping things up. I had some issues getting things to install on my mac without root priveleges. So I did get it working in a github codespace. I noticed the specific dependencies are noted in the manuscript, but they don't seem to be noted in the README, things like the version of R, imager, magick etc. I can raise that as an issue in the repo if you'd like!

image system reqs for an R package are properly placed in DESCRIPTION, that's what it's for.

devSJR commented 1 month ago

@fabian-s Our suggestion is to add a small Trouble Shooting section to the README. Though we pass all tests, we saw that @tijeco had an issue he could get fixed. From experience, R is cross-platform, but the devil is in the details.

@tijeco, if you were so kind and add some technical details (macOS version …) we will add a paragraph like: “biopixR was tested on many platforms and should be functional on all operating systems. However, we have received reports in which users initially had problems with the installation. Under macOS [DETAILS], please check if the following requirements are met …”

Tim (@Brauckhoff) told me that he might get access to a Mac and recheck there, just to be sure.

tijeco commented 1 month ago

@devSJR Ah okay I see it's in the DESCRIPTION file. Perhaps just a small note in the README similar to in the manuscript:

Therefore, biopixR depends on R (≥ 4.2.0), imager, magick and tcltk, imports data.table and cluster

What you have in the github actions is right, and it does work on mac. I was trying to get it on a mac that I don't have sudo access to. I was hoping I could put everything in a conda environment, but imager isn't available through conda for mac (just Linux). I also couldn't install X11 with brew without sudo. I went through a long series of needlessly painful steps all because I didn't have administrator rights. Your package installs perfectly fine in a system that has the ability to install all the necessary dependencies.

devSJR commented 1 month ago

@tijeco I am genuinely sorry but also thankful for your experience. @Brauckhoff got access to a Mac and is now adding this information to the README.

Brauckhoff commented 1 month ago

Hello!

@devSJR I tested the installation on a Mac and found that while the biopixR package installs without issues, loading it can result in an error if X11 is missing. This error occurs due to the imager dependency, which requires X11 to function properly. Without it, the package won't load. Installing XQuartz resolves the issue, and after doing so, everything worked smoothly for me. I tested this on a MacBook running macOS Ventura 13.0 with R version 4.4.1. I added some information to the README to introduce the X11 issue and noted the other dependencies, as suggested by @tijeco.

ColemanRHarris commented 1 month ago

@Brauckhoff I ran into the same issues as @tijeco since I am on an M2 Mac. I will add my thoughts here as I go through my review, but one note on the documentation you added to the README.

The command you listed, brew cask install xquartz, is outdated. When I ran this, I received the following error:

Error: `brew cask` is no longer a `brew` command. Use `brew <command> --cask` instead.

Hence, the proper command should be:

brew install xquartz --cask
tijeco commented 1 month ago

@ColemanRHarris , that's actually what they have in their github actions! https://github.com/Brauckhoff/biopixR/blob/1880095c8bca6701b5363857f1cf4bba9738379b/.github/workflows/R-CMD-check.yml#L58

Brauckhoff commented 1 month ago

@ColemanRHarris, thank you for the correction. I rechecked the documentation in the CI, as mentioned by @tijeco, and I found a minor mistake in the order of commands that could be responsible for the error. The correct command should be brew install --cask xquartz, I will update this in the README.

devSJR commented 1 month ago

@Brauckhoff I ran into the same issues as @tijeco since I am on an M2 Mac. I will add my thoughts here as I go through my review, but one note on the documentation you added to the README.

The command you listed, brew cask install xquartz, is outdated. When I ran this, I received the following error:

Error: `brew cask` is no longer a `brew` command. Use `brew <command> --cask` instead.

Hence, the proper command should be:

brew install xquartz --cask

We certainly need some input here. Linux is our major platform. We try our best regarding Mac.

ColemanRHarris commented 1 month ago

@devSJR Updating the README is sufficient, as that will be useful for any future users with a Mac.

ColemanRHarris commented 1 month ago

Only two notes on the documentation (not sure if this is for @Brauckhoff or @devSJR), otherwise everything looks and works as expected 👍

  1. When loading the biopixR library from R at the command line, I get the following warning. This did not prevent me from running any of the documentation examples, but might be worth investigating this or suppressing the warning.

    Loading required package: tcltk
Warning message:
In fun(libname, pkgname) :
no display name and no $DISPLAY environment variable

  2. Change links to vignettes in the README and elsewhere from https://github.com/Brauckhoff/biopixR/blob/main/vignettes/biopixR.Rmd to https://cran.r-project.org/web/packages/biopixR/vignettes/biopixR.html so that the vignette examples are readable.

Brauckhoff commented 1 month ago

@ColemanRHarris, I updated the URL in the README file as you noted.

Regarding the warning you mentioned, I tried to reproduce it on my console, but I couldn’t replicate the issue. It seems I am still able to access a display environment, even when running R through the console.

To address the warning message, I removed tcltk from the dependencies and moved it to the suggests section. Additionally, I added code to check for a display environment (X11) to ensure that tcltk is only loaded if the requirements are met. This way, we should be able to maintain the same functionality as before.

ColemanRHarris commented 1 month ago

Hi @Brauckhoff, I uninstalled the package and re-installed the current version from GitHub. I do see some of the changes you made, but still get this output when loading the package, e.g. library(biopixR):

Loading required package: magick
Linking to ImageMagick 6.9.12.93
Enabled features: cairo, fontconfig, freetype, heic, lcms, pango, raw, rsvg, webp
Disabled features: fftw, ghostscript, x11
X11 not available, skipping tcltk.
Warning message:
In fun(libname, pkgname) :
  no display name and no $DISPLAY environment variable

I looked through your repository, it seems like you still import tcltk in the NAMESPACE file, have you re-run the build with your current code changes?

Brauckhoff commented 1 month ago

Hi @ColemanRHarris, yes I realized that I forgot to remove the import statement in the corresponding function, which caused it to stay in the roxygen documentation. It should work now, as I have removed the import from both the function and the NAMESPACE.

ColemanRHarris commented 1 month ago

@Brauckhoff Fantastic, thank you! Can confirm that I installed the updated version and produced no warnings! 👍 See output below:

Loading required package: magick
Linking to ImageMagick 6.9.12.93
Enabled features: cairo, fontconfig, freetype, heic, lcms, pango, raw, rsvg, webp
Disabled features: fftw, ghostscript, x11
X11 not available, skipping tcltk.
fabian-s commented 1 month ago

@ColemanRHarris @Brauckhoff @tijeco Thank you all for the collaborative and constructive review process.

fabian-s commented 1 month ago

@editorialbot check references

fabian-s commented 1 month ago

@editorialbot generate pdf

editorialbot commented 1 month ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

✅ OK DOIs

- 10.21105/joss.06394 is OK
- 10.32614/CRAN.package.biopixR is OK
- 10.21037/jlpm.2019.04.05 is OK
- 10.1016/j.biosx.2024.100484 is OK
- 10.32614/rj-2011-002 is OK
- 10.32614/RJ-2012-018 is OK
- 10.32614/CRAN.package.countcolors is OK
- 10.21105/joss.01012 is OK
- 10.32614/CRAN.package.magick is OK
- 10.1201/9781138359444 is OK
- 10.1007/978-0-387-75936-4 is OK
- 10.1109/tsmc.1973.4309314 is OK
- 10.18637/jss.v021.i05 is OK
- 10.32614/CRAN.package.cluster is OK
- 10.1007/s00604-019-3449-y is OK
- 10.1080/17425247.2016.1192122 is OK
- 10.1007/s10544-018-0314-4 is OK
- 10.1007/s00604-014-1243-4 is OK
- 10.1038/76469 is OK
- 10.1007/s11356-020-08127-2 is OK
- 10.1038/s41598-019-41136-x is OK
- 10.1021/ac103277s is OK
- 10.3390/molecules201219766 is OK
- 10.1007/s00216-019-02199-x is OK
- 10.1007/10_2011_132 is OK
- 10.21037/jlpm.2018.11.01 is OK
- 10.21037/jlpm.2018.04.10 is OK
- 10.18637/jss.v049.i09 is OK
- 10.1198/106186007X178663 is OK
- 10.32614/RJ-2015-011 is OK
- 10.5281/ZENODO.12744222 is OK

🟡 SKIP DOIs

- No DOI given, and none found for title: R: A Language and Environment for Statistical Comp...
- No DOI given, and none found for title: Tcl/Tk Interface
- No DOI given, and none found for title: RStudio: Integrated Development Environment for R
- No DOI given, and none found for title: Advanced R
- No DOI given, and none found for title: ’Radiomic’ Image Processing Toolbox
- No DOI given, and none found for title: foodwebr: Visualise Function Dependencies

❌ MISSING DOIs

- None

❌ INVALID DOIs

- None
editorialbot commented 1 month ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

fabian-s commented 1 month ago

@Brauckhoff

At this point could you:

I can then move forward with recommending acceptance of the submission.