Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upbaRcodeR Submission #338
baRcodeR Submission #338
Comments
Editor checks:
Editor commentsThanks for your submission @yihanwu ! Here's the output from goodpractice. You don't need to address these now, it's info for reviewers to use to get started. ── GP baRcodeR ─────────────
It is good practice to
✖ write unit tests for all functions, and all package code in general. 94% of code lines are covered by test cases.
R/hidden_createPDF.R:272:NA
R/hidden_createPDF.R:307:NA
R/hidden_createPDF.R:308:NA
R/hidden_createPDF.R:309:NA
R/hidden_createPDF.R:310:NA
... and 14 more lines
✖ write short and simple functions. These functions have high cyclomatic complexity:custom_create_PDF (58).
✖ not use "Depends" in DESCRIPTION, as it can cause name clashes, and poor interaction with other packages. Use "Imports" instead.
✖ not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import only the specific functions you need.
✖ avoid 'T' and 'F', as they are just variables which are set to the logicals 'TRUE' and 'FALSE' by default, but are not reserved words and hence can
be overwritten by the user. Hence, one should always use 'TRUE' and 'FALSE' for the logicals.
R/hidden_createPDF.R:NA:NA
───────────────Seeking reviewers now Reviewers:
|
sckott
added
2/seeking-reviewer(s)
3/reviewer(s)-assigned
and removed
1/editor-checks
2/seeking-reviewer(s)
labels
|
reviewers are now assigned: @raynamharris & @llrs |
|
Great! Thanks @sckott |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 2 hours
Review Comments
Here is the session info from my workflow. |
|
thanks so much for your review @raynamharris ! |
|
Thank you for the review @raynamharris! |
|
Sorry for the delay. I plan to submit mine in a week or so. I already have most of it written but I want to check more the worflow. |
|
I was looking over the rOpenSci peer review guidelines today (because they are so good and I wanted to use this as a guide for a different peer review that I'm working on), and I came across this bullet point "If you have your own relevant data/problem, work through it with the package. You may find rough edges and use-cases the author didn’t think about." In my current research, I always sample three specific biological tissue that that I code "hyp", "pit' and "gon". Is there a to make barcode with strings in addition to number? So, something like animal-01-hyp, animal-01-pit, animal-01-gon would be awesome. If allowing any random string isn't possible, perhaps something, "A", "B", and "C" could be added so that you could generate sample names like animal-01-A, animal-01-B, animal-01-C instead only only animal-01-tissue-01, animal-01-tissue-02, etc? Does that make sense? |
|
@yihanwu question when you get a chance ^ |
|
Hi @raynamharris, It is possible to add a suffix string with the hierarchical labels. For example, we can do, and the resulting data frame would look like:
The original thought for the hierarchical labels was that tissue types could be coded as numbers but I can see how it would be overly complicated for cases like yours. Just to confirm, you would want something that could generate: Here, each base label is repeated three times for the three ending strings. |
|
awesome! that's exactly what I was thinking of. thanks for clarifying @yihanwu! |
|
On the todo list it goes! |
|
@llrs reminder that your review is due next Tuesday, thanks! |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 7-8h
Review CommentsTest installation First installation took long time (I don't have much of the dependencies of baRcodeR) but the dependencies look right to me. The second install directly from github worked well and fast now that I already have the dependencies Check package integrity There is a note of non standard files found: The WuEtAl_2019_baRcodeR_MEE.pdf seems like an article about the program, which might be better on some preprint archive or another repository but it doesn't belong to the package. I would recommend to delete it from the repository. CODE_OF_CONDUCT.md and the docs folder should be included in .Rbuildignore. There are few tests, and 2 warnings related to introducing NAs by coercion (test_warnings.R, lines 12 and 13) and one about "Digits specified less than max number. Increasing number of digits." in test-label_generation.R line 13. To test for warnings remember that there is the
The shiny app is only tested locally. Maybe this was a recommendation from the CRAN? It reports that the coverage is only 27% I would aim for 90% or more seeing that you want to test also the shiny app and the code. It also warns about using Also it recommends to think about the dependency to qrcodes that is on Depends instead of Imports. I think it is sensible to have it Depends as without it this packages seems useless. However, Check of the metadata The README doesn't have any badge of status, from repostatus.org. See this section Most if not all of the results that the spelling check reports are from examples or names. Consider adding them the dictionary so that they are not reported again. The readme recommends to provide the The DESCRIPTION lists 6 Imports and 1 Depends but the NAMESPACE only imports two package. It also uses Authors@R and Maintainer it is better to select just one (less things to keep up to date). Nice addition of the ORCID numbers. I would recommen to use Check of the documentation The first thing I notice looking at the documentation is a link to another library. I would also have a short section showing how you can use the library on the README.Rmd via the command line (without prompts). Also probably it would be nice to show the final output of the package, currently it seems like the result is a plain data.frame. The section in the README about "Using the RStudio addin" I think it worth to be in its own vignette. There are some man pages (man/cheatsheet.Rd, man/make_labels.Rd) that call library or require on baRcodeR, it is not necessary. It is not recommended to call the system on a man page like in create_PDF.Rd. If you have to, use There is no information about what do the There isn't any example on Also some examples do not run. From create_PDF: example_vector <- as.data.frame(c("ao1", "a02", "a03"))
create_PDF(Labels = example_vector, name = file.path(tempdir(), "example"))
# Warning message:
# In custom_create_PDF(user, Labels, name, type, ErrCorr, Fsz, ...) :
# Cannot find a label column. Using first column as label input.The details section of Check the vignette The image of the workflow is very informative and useful for the user, but I would move it below the overview (as well as the cheatsheet). The first code block of the vignette is to install some packages that are (or should be) automatically installed when the user installs baRcodeR. In the vignette you use Next you explain the workflow using prompts. Prompts are quoted, as they are printed in the terminal I would show them as code by using the three backticks formatting. The next section is again using prompts. I was a bit confused about what is the goal of this section. In the section "By argument" when I ran the code I got a warning: # Warning message:
# In uniqID_hier_maker(hierarchy = hier_list, digits = 1) :
# Digits specified less than max level number. Increasing number of digits for levelIt is mentioned again to store it in a csv. I tried to Check the functionality The functions are very well commented about what do they do. Almost all the functions implement a sort of prompt. The creation of labels seems a bit over-complicated to me. This allows to omit the loop if you have a hierarchy. The GUI addin is nice, but the "Simple ID Code Generation" and the "Hierarchical ID Code Generation" end up creating a labels.csv instead of creating the bar codes. I tried to create a Hierarchical ID Code Generation with the GUI and I end up with an error : That's after creating a label and pressing "Add level". Inspection of the code Whenever possible avoid Also you use In In hidden_createPDF.R you check a class with There are very long functions ( There are also very long lines, consider shorter lines; 253 lines (15%) are > 80 characters long. The You make use of Check the testing The file I don't know how can you test when you interactively ask the user with prompts but that section should be tested too (hidden_createPDF.R). Conclusions Overall the package has its audience it is easy to use and provides a nice functionality for wet-dry researchers. |
|
Thank you @llrs for the in-depth review! I'll have a point to point review posted after I make the changes suggested by @llrs and @raynamharris |
|
Thank you @raynamharris and @llrs for your detailed reviews and suggestions, which significantly improved the package. Below is a point-by-point response to issues raised, with added commit links where appropriate. Overall, we think we were able to address all of the significant concerns, including new test code to provide additional checks, a re-structured README with content on the add-in GUI moved to a separate vignette. We also added new functionality for making ID codes with ending strings as suggested by @raynamharris. (ropensci/baRcodeR@f550757) llrs review:Check package integrityThere is a note of non standard files found: Non-standard files/directories found at top level:
CODE_OF_CONDUCT.md and the docs folder should be included in .Rbuildignore.
There are few tests, and 2 warnings related to introducing NAs by coercion (test_warnings.R, lines 12 and 13) and one about "Digits specified less than max number. Increasing number of digits." in test-label_generation.R line 13. To test for warnings remember that there is the expect_warning function in testthat.
uniqID_hier_maker is only tested for warnings and not for expected output.
The shiny app is only tested locally. Maybe this was a recommendation from the CRAN?
It reports that the coverage is only 27% I would aim for 90% or more seeing that you want to test also the shiny app and the code.
It also warns about using T and F instead of TRUE and FALSE.
Also it recommends to think about the dependency to qrcodes that is on Depends instead of Imports. I think it is sensible to have it Depends as without it this packages seems useless.
However, goodpractie::gp couldn't continue with the analysis due to some error with linter. Probably this is due to some long string of code.
Check of the metadata The README doesn't have any badge of status, from repostatus.org. See this section
Most if not all of the results that the spelling check reports are from examples or names. Consider adding them the dictionary so that they are not reported again.
The readme recommends to provide the sessionInfo, you might be interested on the package reprex, which can help your users to create the minimal reproducible example.
The DESCRIPTION lists 6 Imports and 1 Depends but the NAMESPACE only imports two package.
It also uses Authors@R and Maintainer it is better to select just one (less things to keep up to date). Nice addition of the ORCID numbers.
I would recommen to use usethis::use_tidy_description to set the fields of the DESCRIPTION in a "standard" format.
Check of the documentation The first thing I notice looking at the documentation is a link to another library.
I would also have a short section showing how you can use the library on the README.Rmd via the command line (without prompts). This way the potential user get a grasp of what does the package.
Also probably it would be nice to show the final output of the package, currently it seems like the result is a plain data.frame.
The section in the README about "Using the RStudio addin" I think it worth to be in its own vignette.
There are some man pages (man/cheatsheet.Rd, man/make_labels.Rd) that call library or require on baRcodeR, it is not necessary.
It is not recommended to call the system on a man page like in create_PDF.Rd. If you have to, use system, but system2 would be better because the command is always quoted. So if you want to pass several commands you should call several times to system2.
There is no information about what do the cheatsheet and the make_labels return.
There isn't any example on custom_create_PDF, I would recommend to add one even if they are not run automatically.
Also some examples do not run. From create_PDF: example_vector <- as.data.frame(c("ao1", "a02", "a03")) Warning message:In custom_create_PDF(user, Labels, name, type, ErrCorr, Fsz, ...) :Cannot find a label column. Using first column as label input.The details section of create_PDF has also a "#` " at the beginning.
Check the vignetteThe image of the workflow is very informative and useful for the user, but I would move it below the overview (as well as the cheatsheet).
As a user I would like to know what does the package first and then how can I do it. The first code block of the vignette is to install some packages that are (or should be) automatically installed when the user installs baRcodeR.
In the vignette you use T and F instead of TRUE and FALSE, T and F are not protected, so it might introduce errors, change them to the full name please.
Next you explain the workflow using prompts. Prompts are quoted, as they are printed in the terminal I would show them as code by using the three backticks formatting.
The next section is again using prompts. I was a bit confused about what is the goal of this section.
In the section "By argument" when I ran the code I got a warning: Warning message:In uniqID_hier_maker(hierarchy = hier_list, digits = 1) :Digits specified less than max level number. Increasing number of digits for levelIt is mentioned again to store it in a csv. I tried to create_PDF and I didn't know where the file was created. Apparently it is created on the working directory. Maybe it should be better explained on the help page.
When I used the argument user = TRUE in create_PDF I got a prompt, I introduced the name of the pdf with quotes. Then the resulting file got also quotes. This could be checked and warned to the user as it could create problems.
Check the functionalityThe functions are very well commented about what do they do. Almost all the functions implement a sort of prompt. I am not sure this is actually helpful as then the user need to know which question corresponds with which argument of the function. The creation of labels seems a bit over-complicated to me. We can obtain all the combinations of a group of variables with expand.grid labels <- expand.grid(fixed = "a", variable = 1:5, join = "-",
The GUI addin is nice, but the "Simple ID Code Generation" and the "Hierarchical ID Code Generation" end up creating a labels.csv instead of creating the bar codes. I expected that they would be directly created.
Also there isn't a way to select where the labels.csv is created.
I tried to create a Hierarchical ID Code Generation with the GUI and I end up with an error : Error: Input list has only one level. Did you forget a level or are you sure you are not looking for uniqIDMaker()?
Inspection of the codeWhenever possible avoid sapply(); use vapply(), as sapply will simplify the output which might cause errors. There are 8 cases of this, maybe some of them can be replaced, in some cases you just want to get the first element, maybe you can use unlist instead. Also you use 1:... like 5 times; use seq_len() or seq_along() or simply seq().
In code_128_make() you used T and F replace them for TRUE and FALSE.
In hidden_createPDF.R you check a class with class(), it is better to use the is() function as it will show all the classes it inherits from.
There are very long functions (make_labels_internals, custom_create_PDF and server that are more than 100 lines) try splitting them in shorter functions. Usually functions of less of 50 lines are recommended.
There are also very long lines, consider shorter lines; 253 lines (15%) are > 80 characters long.
The create_PDF and the custom_create_PDF do the same. I would delete create_PDF.
You make use of grDevices:: and other packages but don't declare on the NAMESPACE that you import them.
Check the testing The file uniqID_maker_addin.R is not tested.
I don't know how can you test when you interactively ask the user with prompts but that section should be tested too (hidden_createPDF.R).
ConclusionsOverall the package has its audience it is easy to use and provides a nice functionality for wet-dry researchers. raynamharris reviewPackage ReviewbaRcodeR is an R package that reproducibly creates sample labels. It can be run from the RStudio console or from a GUI Addin. This is the first time I have ever used a GUI or Addin from within RStudio, and it worked very well after restarting R twice. I would recommend this package to my colleagues who do molecular work. Below is a list of minor comments and concerns. In the README, there is a "See also:" link to a similar package called zintr, but it is unclear why a user might want to refer to it or why they should chose one over the other or both. A brief elaboration would be helpful.
Also, it would be useful to state that Zint is a barcode generator and library.
I really like the graphical overview and the cheatsheet. These are both very useful. I also like that your cheatsheet is archived in FigShare with a DOI.
In order for the baRcodeR GUI to appear in my Addins window, I had to restart RStudio. Perhaps add a note that you made need to restart R after successful installation of the package.
The first time I clicked on baRcodeR GUI my R session was aborted. It worked the second time, so maybe add a comment to restart and retry. Also, this is the message that accompanied the first, unsuccessful launch of the GUI and the aborted R session. baRcodeR:::make_labels() When I click "Create Lable.csv" it appeared like nothing happened, but the file was indeed saved in my working directory. A real bonus would be a pop-up window or a message printed to the console that said something like "file save in your working directory", but a simple fix would be to clarify this in the README.
When following the instructions in the "Using the RStudio addin" section, the text explaining the process comes after the example image, much like a figure legend. However, it would be much easier to follow along if the text was above the image instead of below it. Also, there is very little white space between the images and the text. Some level three headers (eg. Simple ID Codes with uniqID_maker and Hierarcical ID codes with uniqID_hier_maker could help distinguish the sections.)
When I click "add level", I keep getting the error message Label Preview Input list has only one level. Did you forget a level or are you sure you are not looking for uniqIDMaker()? I was, however, successfully able to make a hierarchical labels despite the error message. Is there a way to prevent this error message from appearing?
Why is the default font size 2.2 in the GUI? This seems a little small. Also, in the manual, the default size is 5.
I installed the package with devtools::install_github("yihanwu/baRcodeR", build_vignettes = T, dependencies = T) when when I run 1vignette("Using-baRcodeR"), I get the warning message vignette ‘Using-baRcodeR’ not found.
I was able to past the code from the GUI into the command line and regenerate the same pdf. Success!
I'm not sure I understand the error correction description. The L,M,Q,H makes me think it goes form low quality to high quality, but this is conflicts with the 7%-30% damage description. What does damage mean? What do LMQ and H stand for? Please clarify.
I noticed that this repo has been forked from another repo (nine2k/II-I-II-baRcodes-I-II-III)? Do those authors know that this is being submitted as a package? Will they be offered co-authorship?
|
sckott
added
5/awaiting-reviewer(s)-response
and removed
4/review(s)-in-awaiting-changes
labels
|
thanks for your detailed response @yihanwu @llrs and @raynamharris are you happy with the responses/changes made? any further comments? |
|
Great! Many thanks @yihanwu for answering all the points. The package has really improved with all these commits.
7 & other points. Great, sorry to be so repetitive.
9 & 32. I can't say for sure, but it is strange that
for Note that you don't need to first check for the class then check if the class is the one you need and then compare if any is true. For this short loops it might not matter but for other things it can help you to speed up the process. I also think that some warnings can be split in two sentences. One the consequences and another for the advice. For instance, "Linear barcodes created will have bar width smaller than 0.03 inches which may be unreadable by some barcode scanners." |
|
Hi @llrs, I just wanted to get clarifications on some of the points before I start making the changes. These are numbered based on your comments.
This is enormously helpful if I can get it to work with all the possible test cases.
I'm not sure how I can open a new page when needed without having the information from the previous loop regarding where the next barcode needs to go.
|
|
Hi @yihanwu
This is not needed to be accepted but I think it can be helpful if the users needs to print several hundreds of barcodes (or for future tasks you start).
|
|
More updates:
|
|
Great, many thanks |
|
Hi @sckott, @llrs , @raynamharris, Are there any other changes I should review and make? Cheers, |
|
Sorry about the delay - doing a final check now |
|
Approved! Thanks @yihanwu for submitting and @llrs and @raynamharris for your reviews! A few comments:
To-dos:
For MEE:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
|
Hi @sckott, I've made the changes you have suggested but when I go to transfer the repository, Github tells me I "do not have permission to create public repositories". Is there something else I need to do first? |
|
you should have got an email invitation to a github team within ropensci, did you not get that? |
|
Ah, I found it. Completely my fault. The repo transferred successfully. I've added the docs url pointing to docs.ropensci and removed the automatic deployment from Travis to gh-pages. I tried to access the generated docs at https://docs.ropensci.org/baRcodeR. Is there a delay until they should be up? |
|
there was a problem with our registry builder, it should be done soonish, within next day for sure |
|
its up now https://docs.ropensci.org/baRcodeR/ |
Submitting Author: Yihan Wu (@yihanwu) and Robert Colautti (@ColauttiLab)
Repository: yihanwu/baRcodeR
Version submitted:0.1.4
Editor: @sckott
Reviewer 1: @raynamharris
Reviewer 2: @llrs
Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
Reproducibility: baRcodeR facilitates the creation of unique text identifier (ID) codes and scannable barcodes (linear and 2D) that can be printed onto customized sticker labels with consumer-grade printers to facilitate management, tracking, and data collection involving biological samples. Unique identifier codes and scannable barcode labels are produced via reproducible code which reduce transcription errors that arise with hand-written labels and manual entry of label codes. In contrast to commercial software, no proprietary hardware is required, labels can be generated automatically with biologically informative coding, and labels can be created from the command line.
baRcodeR is designed to minimize bookkeeping errors and to assist researchers who need to track, curate, or measure biological samples. It is particularly well-suited for large collaborative projects with complex sampling design and numerous samples shared among researchers. For example, a large biological research project may have subjects organized as population/treatment-by-family/line-by-time – that is, each subject sampled multiple times and representing a specific family group or genetic line from one or more nested experimental populations or treatments. The custom layout options in baRcodeR allow researchers to rapidly generate meaningful identifier codes that capture the hierarchical sampling structure, and then print customizable labels containing the ID codes as human-readable text with corresponding digital linear or 2D barcodes. The command-line interface also facilitates rapid re-creation of a set of generated identifiers with different sized labels or minor changes to identifier codes – for example, to label multiple tissue collection containers and then later to label test tubes for purified extracts, each with the same basic code with a new code corresponding to sample type.
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Presubmission inquiry #336, approved by editor @melvidoni
Technical checks
Confirm each of the following by checking the box. This package:
Publication options
JOSS Options
paper.mdmatching JOSS's requirements with a high-level description in the package root or ininst/.MEE Options
Code of conduct