-
-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Submission: CytoRSuite - Flow Cytometry Analysis Toolkit #281
Comments
thanks for your submission @DillonHammill - @geanders has been assigned as handling editor |
Editor checks:
Editor comments@DillonHammill : My apologies for the delays in getting this out for review! It looks like a very interesting package, and I'm very excited to be serving as an editor. Please let me know throughout the process if you have any questions. Also, I am much less familiar with Bioconductor than I am with the CRAN ecosystem, so I may have some misunderstandings along the way. Please let me know if there's something I've misunderstood, especially related to Bioconductor, for this package. In the next few days, I will be assigning reviewers for the package. In the meantime, here are a few small issues that preliminary checks brought up that you could address in the package: Some tips for reviewers The README for the package's GitHub repository has some very nice instructions for required BioConductor and GitHub dependencies. As a note to reviewers, make sure you read through these, and you can use the code examples in the README to install those dependencies, especially if you clone the Also, it seems that on MacOS, users might need to install XQuartz to allow the
The following issues came up for
I am not completely sure why this error is coming up, because
Reviewer 1: @jacobpwagner |
Also, @DillonHammill, could you add the following badge to your package GitHub repository README to show that it's currently under review at ROpenSci?
|
Thanks for the feedback @geanders. I have addressed the following today:
Comments on
Questions:
Thanks for your help. |
@DillonHammill, thanks for your changes in response to the initial checks! To answer your two questions:
To continue the discussion on one of your comments for the "goodpractices": Comment 4: It's definitely fine to include these package dependencies. This question is whether they could be included as "Imports" in the DESCRIPTION file rather than "Depends"? Here is some guidance on this from Wickham's R Packages:
I'd love to hear your thoughts on whether CytoRSuite is in this category of "building on" flowCore, flowWorkspace, and openCyto rather than just using some tools from them. That could help in determining whether "Depends" or "Imports" is better to use in this case. My apologies on the delays in getting reviewers assigned to this. It's taking a bit of time to find reviewers who will be good fits, but I've got some good leads and am continuing to work on getting two reviewers assigned. |
@geanders, sorry for not getting back to you sooner. CytoRSuite is definitely designed to be used in conjunction with flowCore, flowWorkspace and OpenCyto (as in the above description). In particular, CytoRSuite builds on OpenCyto which depends on both flowCore and flowWorkspace. I written CytoRSuite in such way as to prevent clashes with these other packages. I will start working on a paper for JOSS submission in the meantime. Any news on the reviewer front? |
@DillonHammill, my review is just about ready so I should be able to get it posted either today or tomorrow. |
@DillonHammill : Thanks for the clarification on the Depends / Imports question! We have one reviewer (@jacobpwagner), but I am still looking for the second. |
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: ~5 Review CommentsCytoRSuite adds very helpful interactive interfaces over the core infrastructure of flowCore, flowWorkspace, and openCyto. The point-and-click interface for gate construction ( Additionally, the plots generated by For the most part, everything ran smoothly. However, I ran in to a few minor issues: VignettesYour vignettes cover all of the major functionality of the package and are very helpful.
You can avoid the error by providing a dummy "title" argument, but ultimately
The problem is that the
CodeThe code is generally very readable and if anything over-commented so it was easy
In terms of end result, it seems fine. No gate is added, which is what I'd expect.
I'm guessing this is related to some of the test data you use in the vignettes from
DocumentationYour documentation looks nice and thorough and the links work well for making navigation easy. I verified that all exported methods have documentation with examples and ran a fair amount of the examples to make sure they worked as expected. R CMD check didn't raise any warnings or errors. My only suggestion would be to maybe break up some of the longer "Details" sections in to a few paragraphs to improve readability ( Other than that, the package looks great. I may continue to run it through some testing and I will update this with any issues I run in to. I look forward to seeing what else you will be adding and please feel free to contact me with issues of flowCore/flowWorkspace/openCyto compatibility. |
@jacobpwagner : Thanks so much for this very helpful and thorough review! |
Thanks for taking the time to review CytoRSuite @jacobpwagner. I will post some comments and make the appropriate changes tomorrow. I will also push some new features if you would like to try them out. |
@jacobpwagner thanks again for your help! I have made a number of changes to CytoRSuite over the last couple of days. I have committed these to the Changes:
New features & Improvements:
Comments:
I will start working on a paper for JOSS submission this week. Please let me know if you encounter any additional issues. |
@jacobpwagner : Thanks so much for the quick response to @jacobpwagner comments! A second (and final) reviewer is now assigned (@lmweber), so there will be some comments coming in over the next few weeks from him, as well. |
Thanks @DillonHammill. I'm sorry for the delay as I was out of the office on vacation. I will work with the testing branch for a while and look over the changes to get back to you as soon as possible. |
Thanks @jacobpwagner. I have noticed an issue in offsetting labels in multi-panel plots, I will work on a fix and push the update tomorrow. |
@DillonHammill, here are my comments after the recent changes Testing changes to fix issues raised
depending on the test code in the else clauses of the conditionals on "CytoRSuite_interact" in the method definitions:
Instead, you could just mock the
where now the method just looks like this (no more hard-coded numbers for testing):
Side note: That Other comments
SummaryMost of the issues I raised were addressed. The X11 issue is still there and is the only thing I pointed out that still really needs to be fixed, but the fix is pretty easy as I discussed. It's up to you whether you want to pull the test code out of your methods as I described, but as a general habit it will make it easier for collaborators to work with improving your methods if they do not have test code incorporated in them. |
@jacobpwagner thanks again for your help! I have pushed the fix for I did not know about |
Cool. Yeah, it looks like there are still a few left. I don't know that any of these will cause problems, but it may be safer to switch them all over to
|
Hi @DillonHammill , thank you for submitting this package. My review comments are pasted below: 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: 5 Review CommentsOverall comments: This package provides a set of tools for analyzing flow cytometry data in R, specifically for compensation, interactive gating, and visualization. This fills a clear need in the flow cytometry data analysis community for performing manual gating and related tasks more easily within R, instead of using commercial platforms (currently, tasks such as pre-processing and more automated analysis pipelines are often performed in R, while manual gating and related tasks are usually done using commercial platforms such as Cytobank and FlowJo). The package has extensive documentation, including several vignettes with examples of workflows. The package is also well integrated with existing R tools for analyzing flow cytometry data, including flowCore, flowWorkspace, and openCyto. I think this will be a very useful package for the flow and mass cytometry community, and am happy to have had a chance to look at it in some more detail. However, I ran into several errors and other issues when trying to run the examples in the vignettes. These are described below, along with some more general suggestions. README Currently, the README includes the same content as the index page of the main documentation. By having the same content in two places, there is a risk that they will become out of sync in future. You could consider consolidating this by shortening the README and including some links to the main documentation pages instead. It would also be useful to update the README to include some information on the 'Statement of need' (see checklist above), e.g., mentioning where CytoRSuite fits into the set of available analysis tools, and the intended audience. This is described nicely in the 'Scope' section in the first comment in this issue (8 Feb), so you could re-use some of that text. Scope One related tool that is not mentioned in the 'Scope' section above is iSEE, available from Bioconductor. iSEE is intended more generally for interactive visualization of single-cell data stored in SummarizedExperiment objects, but one of their vignettes shows how to use it for gating mass cytometry data, so there is some overlap there. However, CytoRSuite contains much more specialized functionality for flow cytometry data and builds on existing flow cytometry packages such as flowCore / flowWorkspace / openCyto, so I don't think this overlap reduces the usefulness of CytoRSuite. Both tools provide very useful functionality for users who wish to perform their full analysis pipelines within R, instead of switching to commercial products for the manual gating steps. You could also consider mentioning (either in the README or in the main documentation) something about how this fits in with tools for analyzing higher-dimensional flow and mass cytometry data (e.g., CytoRSuite could be used for initial pre-gating and for visualizations). Installation and checks I installed CytoRSuite on a Mac, following the instructions on the README and main documentation pages. The installation completed without errors. I also ran
Due to this error, I switched to the
These packages are listed in the
The largest files are the
I'm less sure about these; it would be good if they can be addressed, but I don't have any specific advice about how to fix them. Functionality I tested the functionality by trying to run through the examples in the vignettes. However, I ran into a number of problems, which will need to be addressed:
Due to this error, I switched back to the
Also, an additional minor issue was that at first, I didn't realize that right-clicking was required to complete the polygons and continue to the next step of the gating. I think this is mentioned in some of the code comments, but it would be good to mention it more prominently at the start. I am happy to try running these vignette examples again once the errors and warnings have been addressed. Documentation As mentioned above, the package includes extensive vignettes, which is great for new users. The package also includes help files for the individual functions. One suggestion I have for the function help files is regarding places where you have several help files covering very similar content for related functions (e.g., I also ran into a problem with the examples in the first vignette (i.e., the examples in the README and on the index page of the main documentation). These examples begin with the code:
If I understand it correctly, the intention here is that the reader saves some Code quality The code looks well-structured and commented. If you also submit the package to Bioconductor, you will need to check that you meet Bioconductor's guidelines regarding code style. Additional suggestions Bioconductor: I would strongly recommend also submitting this package to Bioconductor. The package already builds on other Bioconductor packages, so this should be feasible. Making the package available through Bioconductor would make it easier to find, and give it extra credibility in the flow cytometry community. However, it would need go through another set of reviews and meet all of Bioconductor's requirements. I ran Publication strategy: In the comments above, you mentioned submitting the package to JOSS. Of course the publication strategy is completely up to you; but I wanted to mention that I don't think this would be the best place for this package. JOSS papers are very short papers describing the purpose of the package only -- which is fine for many packages -- but for this package, I think it would be more useful to have a somewhat longer paper describing some of the key functionality, maybe along with a short workflow, and examples of visualizations. This would help convince flow cytometry users who are currently using commercial platforms (Cytobank, FlowJo, etc) that CytoRSuite could be a good alternative for them. Some possible venues for this type of paper could be the Bioconductor gateway in F1000Research, or more traditional flow cytometry journals like Cytometry Part A. |
Thanks for your review @lmweber! Sorry the I will need a week or so to finish implementing all of these new features and changes. I will merge them back to the |
Ok no worries, just let me know when it is ready to have another look, thanks! |
@lmweber : Thanks so much for your very helpful review! @jacobpwagner : I wanted to briefly check in---it looks like many of your initial concerns were addressed. Are you satisfied that the X11 concern is now addressed, or is that still something to keep an eye on through the remaining revisions? Also, thanks @DillonHammill for your quick and thorough revisions based on these reviews. I will keep an eye out for the revisions in response to @lmweber 's review. |
@geanders, it looks fixed in the |
@DillonHammill I think you've seen that the API for |
Thanks @jacobpwagner that would be great! Sorry for the delay, I am doing a complete overhaul of |
@DillonHammill , see https://github.com/DillonHammill/CytoRSuite/pull/17, which is based off of your |
@DillonHammill : There's no rush, but I just wanted to check in and see if you have a timeline for submitting revisions? |
Sorry I have not been in contact for a while @geanders. Given all the feedback I received from @lmweber and @jacobpwagner (and others) I have completely re-written the entire package. I found a number of bugs that were difficult to track down, so I have spent a lot of time modularizing and re-factoring a lot of the code. This was a lot of work but it was definitely worth it - I have added a number of new features and it is easy to add more features in the future. I am currently working on finalising a couple of important features and then I will tackle the code coverage, continuous integration and documentation for the website. I will do my best to have everything ready as soon as possible. Thanks for patience, I think it will be worth the wait. |
Sounds good. I'll keep an eye on this and be ready to review the reworked package as soon as it's ready. |
Thanks for the update, @DillonHammill ! That all sounds good to me. |
@DillonHammill : I just wanted to check in again. No worries if you're still working on this, but just so you're aware, we typically change the status to "holding" on review Issues where there are longer overhauls, so that it's clear to the editor in chief that there's some longer-term work on the package as he or she reviews all the open issues. Please let me know if you're anticipating that there will still be a while before adding responses on this package, in which case I'll change its status to "holding", or if you anticipate getting things in soon, in which case I'll leave it as-is. |
@geanders : Thanks for checking in! The package is almost complete! I am planning on re-submitting early next week. I am in the process of updating the website with new workflows so that @jacobpwagner and @lmweber have something to follow. I will be testing the package over the weekend and improving the test coverage. If I am happy with the stability of the package I will submit it again next week. Sorry for taking so long, hopefully it will be worth the wait! |
@geanders : Unfortunately, due to recent changes in the underlying data structures in the RGLab dependencies of this package (RGLab/flowWorkspace#294 ), I am going to need some more time to get everything on par with the RGLab suite of packages. Everything is ready on my end, I will just need to make some changes as to how the data is handled internally to ensure that the package behaves as it used to. I don't see a point in re-submitting the package until it is up to date with the latest RGLab packages. I will do my best to make all the necessary changes over the next week or so. Apologies for further delaying this re-submission, but I think it is essential that I make these changes now. I am happy for you to put this hold whilst I get everything ready. I should have mentioned that I have moved the package to a private repo whilst I finalise everything, so it may appear as though I am not making any changes. I will make this repo public when everything is ready. |
@DillonHammill : I am putting the "holding" tag on this submission for the moment, as that will help our editorial team as they keep track of where all the packages are in the review process. We'll change this back as soon as you're ready to resubmit. |
In the interest of reducing load on reviewers and editors as we manage the COVID-19 crisis, rOpenSci is temporarily pausing new submissions for software peer review for 30 days (and possibly longer). Please check back here again after 17 April for updates. In this period new submissions will not be handled, nor new reviewers assigned. Reviews and responses to reviews will be handled on a 'best effort' basis, but no follow-up reminders will be sent. Other rOpenSci community activities continue. We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other. The rOpenSci Editorial Board |
In this period new submissions will not be handled, nor new reviewers assigned.
Reviews and responses to reviews will be handled on a 'best effort' basis, but
no follow-up reminders will be sent. Please check back here again after 25 May when we will be announcing plans to slowly start back up. We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other. The rOpenSci Editorial Board |
Hi, @DillonHammill! I hope you are doing well. We typically check in on packages that are "on hold" about every three months, to see if the author is ready to proceed, and then will usually close the issue if the package has been on hold for a year or longer. I see that we put this package on hold in January, and while ROpenSci reduced it's normal process during this past spring, we are back to a more normal schedule now. Given that, I wanted to check in and see if you would like us to continue to have this review on hold, or if you are ready to move back into an active review? Either is fine, I just wanted to check in. |
Hi @geanders, thanks for checking in! That is great news, I have been waiting to re-submit for a while now. I should be able to re-submit by the end of next week if that is OK? I am working on a couple of other packages that I hope to submit as well. |
Yes, that would be fine. We'll plan to change out of the "holding" tag once you resubmit and let the reviewers know it's ready for them to look at again. |
I'm ready for reviewing whenever! |
Apologies for not getting back to you last week, our group has been working on getting a paper submitted last week so I was bogged down helping with that. I will devote all my time this week to getting this ready for review. I am currently performing my own code review, as the package has become quite large and a lot of the code can be simplified with the use of newly created functions. I am working on reducing package dependencies as well: DillonHammill/CytoExploreR#66. I have also moved all the interactive data editors to their own package https://github.com/DillonHammill/DataEditR which has been submitted to CRAN and ROpenSci #39. This should be available on CRAN soon, so I am working on updating CytoExploreR to remove this code. Some points were brought up during the CRAN review process that are in CytoExploreR as well - so I will try to fix this as well. I have also removed a lot of the unit tests whilst I make all these changes so I will need to add them back once everything has been updated. I know that the annual cytometry conference is on this week (virtually) so I will try and get everything completed early next week. Thanks for your patience, it will be worth the wait! |
Hello @DillonHammill, I am Noam and I'm going to be taking over as handling editor for CytoRSuite from @geanders. I'll take this opportunity to check on your status. I see that this review is in "holding" as you were working on some considerable changes last year before starting review. Do you have any updates? |
@noamross, thanks for checking in! Due to COVID19 CytoRSuite (now called CytoExploreR) has become the focus of my PhD project. I am working hard to submit my PhD by the end of June and I will submit the finalised version of the package as soon as I am ready. |
Thanks for the update @DillonHammill. Note that with an extended hiatus we may need to find new reviewers for a future version, and especially if you are making big changes, it may effectively be re-reviewed. |
Hi @DillonHammill ! We are doing a sweep of stale review issues. Since this review has been open and inactive for so long, much may have changed including author, editor, and reviewer bandwidth and ever-evolving rOpenSci best practices. As such, I'm closing this issue. If you still have interest and capacity, we would welcome you to open a new submission issue! |
Submitting Author Name: Dillon Hammill
Due date for @jacobpwagner: 2019-04-22Submitting Author Github Handle: @DillonHammill
Repository: https://github.com/DillonHammill/CytoRSuite
Version submitted: 0.9.9
Editor: @geanders
Reviewers: @jacobpwagner, @lmweber
Due date for @lmweber: 2019-04-22
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):
CytoRSuite uses the existing flow cytometry infrastructure (flowCore and flowWorkspace packages) to import flow cytometry data. CytoRSuite (which is built on using the openCyto package) provides the toolkit necessary to interactively manipulate, process and visualise this flow cytometry data.
CytoRSuite is primarily targeted to scientists who traditionally utilise software with a GUI to manually gate their flow cytometry data. CytoRSuite aims to form a bridge between this GUI software and R by providing a similar user experience with minimal coding. Furthermore, CytoRSuite aims to provide a means by which manual gating users can be exposed to and eased into automated gating approaches.
Although other R packages which support automated gating approaches exist, to the best of my knowledge there is no R package which supports manual gating. CytoRSuite not only brings the ability to manually draw gates, but also the potential to combine automated and manual gating approaches through the openCyto gatingTemplate. Furthermore, CytoRSuite expands the existing compensation tools by providing an interactive shiny application to fine tune spillover matrix values in realtime, Additionally, CytoRSuite contains an intuitive set of plotting functions which can provide powerful visualisations with minimal coding. CytoRSuite integrates all the tools necessary for the analysis of flow cytometry data in a single cohesive package.
Technical checks
Confirm each of the following by checking the box. This package:
Publication options
JOSS Options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: