-
Notifications
You must be signed in to change notification settings - Fork 35
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
OpenOmics: Library for integration of multi-omics, annotation, and interaction data #31
Comments
welcome to pyopensci @JonnyTran someone will followup with you in the next week or so! Our progress is slow right now as we're working o n funding for this effort!! @NickleDave was this one you wanted to work on? you are doing a lot so please let me know if you have time (or if you don't that is understandable too!) |
@JonnyTran this looks perfect, thank you. Hey @lwasser yes I have this on my to-do list and will start looking for reviewers Wednesday |
wonderful! @NickleDave thank you! |
Hi all, just adding editor checks. Thank you @JonnyTran for your detailed submission Editor checks:
submitter did not check "yes" for submitting to JOSS
Editor commentsOverall:
Minor comments:
Reviewers: @gawbul @ksielemann |
started reaching out to reviewers, will update as soon as we hear back |
Thanks so much for the great suggestions, @NickleDave. I've addressed the "releases DOI" and "docs link" issues above, and I'm planning on updating the README file to be more attractive to potential contributors. |
Hi @NickleDave and @lwasser, will it be possible for OpenOmics to make submission to JOSS at this stage of the review? I just changed my mind about this, but I already have a complete manuscript and I'm ready to provide the |
Hey @JonnyTran I think that could be okay. But first let me make sure I understand what you're asking.
Do you mean that you want to change "Publication Options" in your submission above, and check the box for "automatically submit to" JOSS?
Do you mean that you have a complete, separate manuscript written about OpenOmics, in addition to the If that's the case, we should discuss more whether you want to submit to JOSS. We can perhaps tag some editors and ask if they can give us input. My impression is that JOSS is usually meant to provide a mechanism for getting publication credit for software in cases where the developer/maintainer can't easily publish a paper about it. so: |
Hi @JonnyTran just want to follow up on this -- please let me know what you're thinking I think I do have one potential reviewer and can move ahead with finding another whenever you're ready |
Hi @NickleDave, sorry it took awhile to consult with my advisor. Thank you for the clarifications! |
great, glad to hear it @JonnyTran please do go ahead and edit your initial comment to check that automatic submission box, and make sure you address the to-do list in that section I will continue with my part of the review process |
Hi again @JonnyTran excited to let you know that @gawbul and @ksielemann have both kindly accepted our invitation to review @gawbul actually started PyOpenSci back in 2013 (see this ROpenSci blogpost) and develops related tools such as pyEnsemblREST @ksielemann has significant experience with omics datasets and was recommended to us by @bpucker as developer of the tool QUOD (from their publication https://www.biorxiv.org/content/10.1101/2020.04.28.065714v1.abstract) @gawbul and @ksielemann here are related links again for your convenience: I will update my editor checks above to add you both as reviewers, and set an initial due date of three weeks: February 15, 2021 |
this is so awesome!! @gawbul hello again!! so great to see you here. We are moving forward with PyOS through the Sloan foundation (fingers crossed) as we briefly discussed forever ago. I'd love to see you continue to participate when you have time in whatever capacity you have time for!! :) thank you all for this review! |
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:
Readme requirements
The README should include, from top to bottom:
UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Functionality
For packages co-submitting to JOSS
Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted. The package contains a
Final approval (post-review)
Estimated hours spent reviewing: 5 Review CommentsThe concept of OpenOmics seems interesting and useful for the integration of various omics datasets. Overall, the package has a clear documentation. However, there are still a few issues that should be addressed. Please see the points above and below.
Is there a possibility to choose the directory in which the files should be downloaded? This would be great. |
Thanks so much for the fantastic reviews @ksielemann! I can work on the revisions, which should be available in 2 weeks.
Thanks for pointing this out and providing the automated scripts. I initially placed the test data at
I used the package |
Just echoing @JonnyTran -- yes thank you for getting this detailed review back so quickly @ksielemann Looks great to me. I will read in detail this weekend just to make sure I'm staying up to date with the review process |
Just working through my review at present. Should have it done by the end of the day. Apologies for the delay. |
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:
So the package states it is solving the problem of targeting various multi-omics datasets, though this is fairly broad, perhaps because the intention is to make the scope of the project broader in the future. It isn't clear what datasets and data formats are supported, however. Perhaps this isn't relevant for the README, but I feel it could be included in the linked documentation on Read the Docs. The target audience is implicitly defined and my assumption is that this would primarily be used by bioinformaticians, though perhaps this could be more explicit?
Installation instructions seemed clear and I attempted to install via
I needed to run the following to fix the issue:
I tried with brew install llvm (version 11.0.1 and it failed with Perhaps an external dependency on this can be specified (required by llvmlite)? The assumption is that the end-user has a working python installation and relevant compilers etc. installed, though this doesn't seem to be specified anywhere?
When trying to run an
This turned out to be an issue with python 3.9, which the package is supposed to support. I tried with python 3.8 instead, but it failed initially with the scipy install as it needed the BLAS and LAPACK libraries. I needed to install using:
Now running
Running the following rectifies this:
I feel, therefore, a dependency on python 3.8 should be specified in the documentation and setup.py, as it looks like there are issues with python 3.9 at present. It would also be useful to include tensorflow in the list of package dependencies (i.e. requirements.txt) to avoid this warning. Using something like pipenv might be an ideal solution here? Though explicitly stating the external library dependencies for scipy would still be necessary. When extending
This is because there are close parentheses where there shouldn't be. The example in the README should be the following (I have submitted a PR for this):
Running
I believe this can be rectified by updating transcriptomics.py here with:
The
Using
Running
This differs from the output in the README, however, which is:
Running the example under Annotate LncRNAs with GENCODE genomic annotations returns the following:
I initially submitted an issue with I was unable to continue with the rest of the vignettes as a result despite attempting some fixes locally.
Docstrings seem to be available throughout the codebase for all relevant functions, however, I found the documentation on Read the Docs to be lacking in comparison, particularly in providing detail of function parameters that are otherwise available in the docstrings.
Looking at the various classes and respective functions throughout the codebase, there doesn't seem to be a full coverage of of all functionality in the examples provided in the documentation. This would be too much for the README, I feel, but could certainly be represented on Read the Docs.
There is no link to the contribution guidelines from the main README, which I feel would be beneficial, however contributing guidelines are available in https://github.com/BioMeCIS-Lab/OpenOmics/blob/master/CONTRIBUTING.rst and on Read the Docs. Just being finicky, I personally would prefer a common format for the README and CONTRIBUTING documents etc. Either both in reStructuredText or both in Markdown. There seems to be an outdated README.rst that could probably be replaced with the current README.md?
Readme requirements
The README should include, from top to bottom:
I feel some things are missing regarding setup information for certain platforms (i.e. I had issues on macOS). Use of
No comparisons are made to other packages. I feel it is similar in some ways to PyCogent, though this is no longer actively maintained, and perhaps even comparable in some ways to QIIME2?
No citation information is provided in the README, though perhaps this can be made available if submitted to JOSS. It would also be great to see a DOI made available via GitHub's integration with Zenodo as mentioned by @NickleDave. UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. One minor PR submitted as described above. I'll try see if I can spare some time to look into the
Though could be extended as discussed above.
This is relatively clear, but could be expanded on as above.
As discussed above, there appear to be docstrings throughout the code, but this doesn't seem to be reflected in the README or online documentation, particularly for function parameters. Functionality
I had several issues with installation as described above, though I only tested on a macOS system running Big Sur version 11.2. It appears there are issues with Python 3.9.x in particular, though I can't confirm whether this is platform specific. There were some failures in building scipy on Python 3.8.7 due to dependent libraries missing on my machine.
The package goes a good way towards meeting the claims it reports to be developed for, though the difficulties in getting the examples to run means I was limited in my ability to fully assess them.
No performance claims were provided. On my 16" MacBook Pro with 2.3 GHz Octa-core Intel Core i9 and 32GB RAM, however, I felt the package was a little slow in loading it's dependencies on first run. Tests also took some time to complete.
Some tests are available and run as part of the Travis CI pipeline, though coverage isn't amazing and would benefit from additional work. Focusing on test driven development is a good way to ensure greater coverage. Running the tests locally took a long time, and returned various warnings and an error.
The main error seemed to be a missing dataset. On further inspection of the codebase it seems that the package is supposed to download the miRTarBase data (although it appears to have the version 7 release URL hardcoded when version 8 is now available). I wondered whether this was a permissions issue with not being able to create the
This is likely due to macOS system integrity protection. However, it seems I am also unable to resolve http://mirtarbase.mbc.nctu.edu.tw/cache/download/7.0/. I believe the URL should actually be http://mirtarbase.cuhk.edu.cn/cache/download/7.0/ (or even http://mirtarbase.cuhk.edu.cn/cache/download/8.0/)? I manually updated to the working version 7.0 release and updated the path in
I still received the error, so something needs looking at in more detail here.
When raising a PR the build in Travis CI failed for all python versions. There is some work needed to get these issues resolved though I didn't inspect the output in detail.
Final approval (post-review)
A number of changes and bug fixes are required before I would recommend approving this package, but in general I feel it would be a great addition to pyOpenSci. Estimated hours spent reviewing: 7 Review CommentsI would recommend looking through the Author's Guide in more detail, particularly the Tools for developers section. Using git pre-commit hooks for local development would be beneficial for both the author and any contributors and would enable the production of greater quality code. These can also be integrated with Travis CI to ensure any pull requests also meet the same requirements via automated testing against a variety of different Python versions. I echo the comments of @ksielemann, in that the package seems very interesting and I can see it would have a broad range of applications. Hopefully, we can work together to get the issues resolved and get this approved. Would be interested in seeing this in JOSS at some point too. |
Just in case it gets lost in the review, I submitted PRs for a couple of the issues I hit here JonnyTran/OpenOmics#103 and here JonnyTran/OpenOmics#105. Also opened an issue for the problem with the |
Thank you @gawbul for the very thorough review, really appreciate that you could get that back to us a week before the deadline, especially with other things you've been dealing with. @JonnyTran just want to check where you are at with this. I know it might feel like a lot, and you could have other things going on. Our guidelines suggest aiming for a two-week turnaround time after reviews are in. We definitely don't have to hold strictly to that especially if you have other obligations to deal with. But please when you can just give me some idea of how you'll move forward. I would suggest converting reviewer comments into issues on OpenOmics and linking to them where you can. See for example issues on |
Thanks so much for everyone's patience, help and support, @NickleDave, @lwasser, @gawbul, and @ksielemann ! Most of all thanks for making this a better software! I will work on making it more usable for all. |
oh all - So the JOSS review process is simple in that they accept our review as theirs! @arfon please note that this package was submitted to the JOSS review process. Can we please fast track it given we have reviewed here on the pyopensci side of things. Please let us know what you need. @JonnyTran is there an open issue in JOSS right now? can you kindly reference it here if you haven't already. I normally keep these reviews open here until the JOSS part is finished. When it is they will ask you to add the JOSS badge on your readme as well. thank you all for this! |
Hi @lwasser , there isn't a JOSS issue yet, but I will tag this pyOpenSci issue once it is opened. |
ahh ok perfect. Normally they just accept through our issue! let's wait for arfon to get back to us here to ensure that is still the best process! congratulations on being a part of the pyopensci ecosystem and thank you for your submission here! |
Sure, I am okay with this! Should I add myself to the file you mentioned above or do you prefer to add me to the contributor site yourself? |
Sorry for the delay folks. Things are now moving in openjournals/joss-reviews#3249 |
@ksielemann it would be excellent if you could add yourself with a PR
Great thank you @arfon !!! |
This is now published in JOSS https://joss.theoj.org/papers/10.21105/joss.03249 🥳 |
yea!! thanks so much @arfon and thank you for the reviews @gawbul @ksielemann and the editor effort @NickleDave closing this!! |
hey 👋 @JonnyTran @gawbul @ksielemann @NickleDave ! I hope that you are all well. I am reaching out here to all reviewers and maintainers about pyOpenSci now that i am working full time on the project (read more here). We have a survey that we'd like for you to fill out so we can:
NOTE: this is different from the form designed for reviewers to sign up to review. Thank you in advance for doing this and supporting pyOpenSci. |
Hi there 👋 @JonnyTran @ksielemann I know that everyone is super busy BUT if you have just 5-10 minutes to fill our our onboarding / feedback survey for this review i'd greatly appreciate it!! Many thanks in advance for your time. it really helps our organization! Jonny, if there are other maintainers involved in this package please ping them here as well. we would like to keep in touch with our maintainers to ensure the package is still being kept up to date AND to understand how we can better support you ! Many thanks in advance!! |
Thank you for the reminder! I've just submitted the survey. Looking forward to future contributions to PyOpenSci |
Thank you @JonnyTran, same! |
you rock @JonnyTran !!! |
Submitting Author: Jonny Tran (@JonnyTran)
All current maintainers: @JonnyTran
Package Name: openomics
One-Line Description of Package: Library for integration of multi-omics, annotation, and interaction data
Repository Link: https://github.com/JonnyTran/OpenOmics
Version submitted: 0.8.4
Editor: @NickleDave
Reviewer 1: @gawbul
Reviewer 2: @ksielemann
Archive:
JOSS DOI:
Version accepted: v 0.8.8
Date accepted (month/day/year): 04/17/2021
Description
OpenOmics is a Python library to assist integration of heterogeneous multi-omics bioinformatics data. By providing an API of data manipulation tools as well as a web interface (WIP), OpenOmics facilitates the common coding tasks when preparing data for bioinformatics analysis. It features support for:
OpenOmics also has an efficient data pipeline that bridges the popular data manipulation Pandas library and Dask distributed processing to address the following use cases:
Scope
* Please fill out a pre-submission inquiry before submitting a data visualization package. For more info, see notes on categories of our guidebook.
@tag
the editor you contacted:Technical checks
For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:
Publication options
JOSS Checks
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Note: Do not submit your package separately to JOSS
Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?
This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.
Code of conduct
P.S. *Have feedback/comments about our review process? Leave a comment here
Editor and Review Templates
Editor and review templates can be found here
The text was updated successfully, but these errors were encountered: