search

openjournals / joss-reviews

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

[REVIEW]: Translational Informatics Management System (TIMS): Towards OMICS based clinical data management for long term curation of clinical studies #1533

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @absd@bii.a-star.edu.sg (Wing-Cheong Wong) Repository: https://github.com/bii-absd/tims Version: v2.05 Editor: @csoneson Reviewer: @rabdill, @andreysmelter Archive: 10.5281/zenodo.3357938

Status

status

Status badge code:

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

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

@rabdill & @andreysmelter, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

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

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @rabdill

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @andreysmelter

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @rabdill, @andreysmelter it looks like you're currently assigned to review this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

csoneson commented 5 years ago

@rabdill, @andreysmelter - please perform your reviews in this issue (see instructions above). Don't hesitate to ping me if you have questions.

csoneson commented 5 years ago

Hi @bii-absd - to simplify the review process, please take a look at the review checklist above, and make sure that your submission is as complete as possible according to these criteria already at this stage. Thanks!

bii-absd commented 5 years ago

Dear all, I believe someone hit an error while trying to export data to cBioPortal on TIMS (we received the error email). This error is caused by the firewall not being open for local connection to port 3306 (mysql) when our IT guys setup the system for public access. The issue is resolved now, and sorry for the inconvenience caused.

@rabdill - Working on the community guidelines. Sorry for the oversight.

rabdill commented 5 years ago

Hi @bii-absd, that was me who hit the error, thanks for fixing it so quickly. No need to apologize about the community guidelines, just wanted to mention it.

csoneson commented 5 years ago

👋 @rabdill, @andreysmelter - just checking in on the reviews. Do you think you will be able to provide your feedback to @bii-absd during the coming week? Thanks!

rabdill commented 5 years ago

Hi, my apologies for the delay. I'm able to check off most of the review items confidently, but there are a few points still to address. I'm not sure if I should put them here or create issues in the repo itself. In order of importance:

Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified? This requirement appears to be entirely absent, currently. There are no automated tests I can find, and the only manual testing is just the descriptions of example operations that users can perform in the quick start guide. This doesn't seem sufficient for this requirement.

Authorship: Has the submitting author (@absd@bii.a-star.edu.sg) made major contributions to the software? Does the full list of paper authors seem appropriate and complete? It's difficult to determine what the submitting author contributed to the repository itself because almost all their commit messages are the same, "Add files via upload," mostly related to documentation. It appears the application code is (almost?) exclusively written by the paper's first author. This doesn't seem unreasonable, but, based on the software itself, it's not technically clear what the submitting author did. Not sure how to address this one, @csoneson. Is it acceptable, when the submitting author is also the last author, for them to have taken a more supervisory role than one in which they were literally writing the software?

Installation: Does installation proceed as outlined in the documentation? I'm going to need some time to figure out how to verify this one—the minimum requirements are difficult to pull together:

Intel i7 processor, 32GB of RAM and at least 500GB of storage space on Ubuntu Linux LTS 16.0.4.

I'm assuming, if I don't attempt to perform lots of actual analysis locally, that I'll be able to at least install the web application with less resources. I'll be able to work on that this week.

Functionality: Have the functional claims of the software been confirmed? Also something I need to make progress on. The online demo is very helpful in this regard.

Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support Submitted an issue about this a couple weeks ago, and the submitting author said they were working on this bit.

Version: Does the release version given match the GitHub release (v1.0)? There is not currently a v1.0 release in the repository, but assuming there will be some more changes it seems like this could be the final step. Very minor.

csoneson commented 5 years ago

Thanks for your comments @rabdill!

Is it acceptable, when the submitting author is also the last author, for them to have taken a more supervisory role than one in which they were literally writing the software?

Yes, a supervisory role is certainly fine. We currently don't require a formal author contribution statement, and authors are ultimately responsible for deciding which contributions are sufficient for co-authorship (see also https://joss.readthedocs.io/en/latest/review_criteria.html#authorship). In cases where contributions are not clear from the commit history, we can also ask for a clarification from the submitting author.

bii-absd commented 5 years ago

@rabdill - Thanks for your comments!

About automated/manual tests: Currently, we provide the OMICS datasets for manually testing TIMS software based on the instructions from the quick start guide and user manual.

About authorship: absd@bii.a-star.edu.sg is a mailing list that comprises of all the authors in the TIMS paper. Mainly, Weihong Tay (S/W engineer) wrote the TIMS software; Erwin tantoso (Bioinformatician) adapted the open-sourced OMICS pipelines for integration into TIMS; Wing-Cheong Wong (Principal Investigator) conceptualized and designed the TIMS software.

About Installation: The resource needed for TIMS (without the OMICs pipelines due to large genomic databases) is rather modest; Please comment out the last 4 lines of the "TIMS_installation.sh" to exclude the pipeline installation.

About the community guidelines: We have uploaded the file - community-guidelines.md

rabdill commented 5 years ago

@bii-absd this is very helpful, thank you.

csoneson commented 5 years ago

ping @andreysmelter

👋 just checking in on the reviews. Do you think you will be able to provide your feedback to @bii-absd during the coming week? Thanks!

andreysmelter commented 5 years ago

Hi @csoneson, @bii-absd, @rabdill, I appologize for the delay, here are my review comments:

General checks

Version: Does the release version given match the GitHub release (v1.0)?

I was able to install the TIMS on Ubuntu 16.04.6 and the version appers to be 2.05. I did not find release version within the GitHub repo. Although TIMS-Manual-release-1.0.pdf says that this is Release 1.0.

Authorship: Does the full list of paper authors seem appropriate and complete?

This seems to be resolved above by @rabdill

Functionality

Installation: Does installation proceed as outlined in the documentation?

I followed the steps provided in the TIMS-Automated-Installation-Guide-V0.7.pdf, commented out the last 3 lines as suggested to prevent installation of pipelines (Perl, Python, and R packages). I was able to install the TIMS but run into several issues.

Few issues that I run into:

Functionality: Have the functional claims of the software been confirmed?

Documentation

Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

The repo provides manual and quick-start guide in pdf format with examples and screenshots. There is no automatically generated functionality API documentation of the software but this may not be necessary for this type of software/publication.

Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?

As mentioned @rabdill there is no automated testing found in the repo. The installation guide provides a link to the dataset for quick manual testing. But the link to the testing dataset appears to be broken. Again, this manual testing may be sufficient for this type of software/publication.

Community guidelines

The community-guidelines.md file is in the repo now. I think I would suggest renaming it into more standard CONTRIBUTING.md and reference it in the README.md file. This issue https://github.com/bii-absd/tims/issues/2 can be closed.

rabdill commented 5 years ago

The only sticking point, for me at least, is the lack of testing, but I am a little unclear about what the requirement is—would you be able to clarify, @csoneson?

Short version: Currently, the only form of testing available for TIMS is extensive user documentation that describes intended behavior. This is helpful for reviewers, but not for future contributors, who will have no systematic way to validate code changes. I don't know if that is actually required for acceptance.

Much longer version: According to the review criteria, the "not acceptable" category is listed only as "No way for you the reviewer to objectively assess whether the software works" (emphasis mine). However, the wording of the review checklist is a little different, asking more broadly about whether "the function of the software can be verified."

I'm sorry if I'm being annoying by parsing the words too closely, but I guess the distinction I'm getting hung up on is this: If "testing" is only required for reviewers to do an eyeball check for whether the software currently performs as described, that bar is relatively low, but satisfied by this submission—the authors state above that "we provide the OMICS datasets for manually testing TIMS software based on the instructions from the quick start guide and user manual," which is true. However, if the required testing is intended to help future contributors validate that their changes didn't break anything important, I don't think the current testing is sufficient for acceptance.

The main benefit of TIMS is that it integrates so many different systems in a helpful way. However, this also presents lots of opportunities for unintended consequences to be introduced by future development. Even if we skip over the actual pipeline workflow, there are still lots of other important features: user management, permissions, file management, security auditing, and so on. Currently, the only way to validate the functionality of these interwoven systems is to click around and check to see whether it looks right. That doesn't seem to me like it should count as "testing," BUT I'm not sure if that level of testing is required by JOSS.

TIMS is a strong submission, and I can certainly sympathize with the amount of time it takes to add and maintain automated testing for a complex project like this. I'm just not clear on whether tests like that are required for acceptance. Thanks.

bii-absd commented 5 years ago

@andreysmelter - Many thanks for your comments!

Installation : (i) There are some pipelines (external packages) that does not run correctly on Ubuntu LTS 18.04 but fine on LTS 16.0.4, hence installation script checks to ensure pipeline compatibility. (ii) As per your suggestion, we will rewrite the installation guide using markdown.

Functionality: (i) My sincere apologies, I have updated the installation guide with the correct link - http://mendel.bii.a-star.edu.sg/SEQUENCES/TIMS/Downloads/TestData.tar. Meanwhile, we will convert the guide into markdown. (ii) As per your suggestion, we will upload the test datasets (~3.5GB) into FigShare(limit of 5GB) so it remains publicly available.

Community guidelines: The community-guidelines.md file has been changed to CONTRIBUTING.md and referenced in README.md file.

bii-absd commented 5 years ago

@rabdill - Once again, thanks for your comments!

It is in our understanding from the review criteria that "Authors are strongly encouraged to include an automated test suite covering the core functionality of their software". Hence, an automated test suite does not seem to be mandatory. However, from the perspective of good software engineering practice, your comments are reasonable.

Admittedly, TIMS is quite a complex or rather ambitious project for a small team of 1 software engineer and 1 bioinformatician (not of formal computer science background) in an academic setting. In our current environment, we have more testers (the actual users themselves from hospitals/diagnostic labs with no programming background) than software engineers, hence we have to opt for the manual testing route as the practical option.

In all honesty, we would love to develop an automated test suite but this requires testers who can code these automated tests (automation engineers or developers). Also, given the complexity of TIMS, a proper test suite should encompass the automated tests at multi-levels : UI (user interface), API and unit testing. At least for now, this is quite formidable task to us.

csoneson commented 5 years ago

@rabdill - thanks for raising an important question about the testing. Currently we don't require an automated testing infrastructure (although it is encouraged). However, it is important that there is a clearly documented, objective way to test the functionality of the software. For example, a test data set, clear instructions on how to run it through the software to test all the relevant functionality, and what values to expect as the output at each step. This would also help new contributors (and the authors, in order to decide whether to merge a pull request from a contributor), to make sure that nothing is broken.

Only having, for example, a set of files that can be used to run the software and "click around" is not enough, since it assumes that the tester knows what to expect the output to be (and thus is rather subjective than objective). It also doesn't guarantee that all relevant aspects are being tested.

rabdill commented 5 years ago

@bii-absd Thank you for clarifying. I was able to complete the installation as described, but there's one last issue preventing me from ticking the last "Functionality" box. I'm able to perform operations in every category on the home dashboard except for one: The integration with cBioPortal appears to still not be working properly. This initially came up a few weeks ago, and I tried in three different browsers in case I had cached something weird. In both provided examples in the "Visualize my data" section, clicking on the "Launch" button opens a new tab with the URL http://172.20.192.236:8080/cbioportal/ and a sid URL parameter, but this request times out every time.

(One minor note I happened to notice that it appears the "from" field in TIMS emails is hard-coded to come from the original project, "TIMS@bii.a-star.edu.sg"—I just thought I'd mention it in case any TIMS people were bothered by it.

bii-absd commented 5 years ago

@rabdill https://github.com/rabdill Thank you for the feedback, the issue reported on the integration with cBioPortal has been resolved.

On 30/07/2019 05:52, Rich Abdill wrote:

@bii-absd https://github.com/bii-absd @csoneson https://github.com/csoneson Thank you for clarifying. I was able to complete the installation as described, but there's one last issue preventing me from ticking the last "Functionality" box. I'm able to perform operations in every category on the home dashboard except for one: The integration with cBioPortal appears to still not be working properly. This initially came up a few weeks ago https://github.com/openjournals/joss-reviews/issues/1533#issuecomment-508814312, and I tried in three different browsers in case I had cached something weird. In both provided examples in the "Visualize my data" section, clicking on the "Launch" button opens a new tab with the URL |http://172.20.192.236:8080/cbioportal/| and a |sid| URL parameter, but this request times out every time.

(One minor note I happened to notice that it appears the "from" field in TIMS emails is hard-coded to come from the original project https://github.com/bii-absd/tims/blob/tims_agpl3/src/java/TIMS/General/Postman.java#L46, "TIMS@bii.a-star.edu.sg mailto:TIMS@bii.a-star.edu.sg"—I just thought I'd mention it in case any TIMS people were bothered by it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1533?email_source=notifications&email_token=AHAIJNKRPIWQ62XJO55KXYDQB5RANA5CNFSM4H4K7D2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3CDVCY#issuecomment-516176523, or mute the thread https://github.com/notifications/unsubscribe-auth/AHAIJNM6EN4CX3XXBVT63LLQB5RANANCNFSM4H4K7D2A.

rabdill commented 5 years ago

Thanks so much for your prompt response. I'm now comfortable signing off on my review, @csoneson. The repository itself still needs a "1.0" release, but it makes sense for that to be the last step of the process.

csoneson commented 5 years ago

Thanks @rabdill - yes, the version can be set as the last step. I will ping you again when we're there, to get a completely ticked off review list :)

bii-absd commented 5 years ago

@rabdill - Thanks for taking the time to do the review. We greatly appreciate your help.

@andreysmelter - (i) The test dataset has been uploaded to FigShare and (ii) a markdown version of the installation guide has been created in the 'Installation' folder.

andreysmelter commented 5 years ago

@bii-absd, thank you for the prompt response, I checked of my reviewer list for the functionality docs and tests.

There is still seems to be some confusion regarding the release version information - it seems to be set arbitrary (the web-interface says it is version 2.05 and the documentation says it is Release 1.0). @bii-absd one simple way to add the correct version is to make sure that the documentation and web interface say the same version and also create a release artifact. This is simple to do since it is under GitHub - go to "releases" link (https://github.com/bii-absd/tims/releases) and create new tagged release with the specific version that you want to - this will match then to the exact point in time that the JOSS paper describes.

@csoneson I feel comfortable signing of my review list as well.

Thank you!

csoneson commented 5 years ago

@andreysmelter, @rabdill - thanks a lot for your thorough reviews!

@bii-absd - I'll take another quick look at your submission and get back to you shortly. At this stage, please make sure that the version information is consistently reported so that the reviewers can tick off the last boxes in the checklists.

csoneson commented 5 years ago

@whedon check references

whedon commented 5 years ago 
Attempting to check references...
whedon commented 5 years ago 

OK DOIs

- 10.1080/14636778.2017.1370671 is OK
- 10.1186/s13059-016-0917-0 is OK
- 10.1073/pnas.1708272114 is OK
- 10.1093/bib/bbs064 is OK

MISSING DOIs

- None

INVALID DOIs

- None
csoneson commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

csoneson commented 5 years ago

@bii-absd - I made a PR with some small formatting fixes to the paper. Please review and merge, and generate a new pdf proof.

Next, please make sure that the version information is consistently reported, and create a tagged release of your software. Then create a Zenodo archive, making sure that the title and authors of the archive are exactly the same as in the paper. Once that's done, please report back here with the version and the DOI of the Zenodo archive.

bii-absd commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

bii-absd commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

bii-absd commented 5 years ago

@andreysmelter - Thank you for all your time committed to this review! We are really grateful for your help. As per your suggestion, we have tagged the release of TIMS software to v2.05. We apologized for the confusion caused due to the inconsistencies between our manuals and software versioning. As such, we have renamed all the TIMS's quick guide, full manual and installation notes to version 2.05 for consistency.

@csoneson - Thank you for handing our submission! We have ensured that : (i) the version information between software and documentation is consistent with the creation of a tagged release of our TIMS software to v2.05. (ii) the JOSS paper has been proofread and regenerated. (iii) the Zenodo archive has been created at https://zenodo.org/record/3357938; the Zenodo generated doi is : 10.5281/zenodo.3357938.

Please let us know if anything else is needed. Thank you.

csoneson commented 5 years ago

@whedon set 10.5281/zenodo.3357938 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.3357938 is the archive.

csoneson commented 5 years ago

@whedon set v2.05 as version

whedon commented 5 years ago

OK. v2.05 is the version.

csoneson commented 5 years ago

@bii-absd - great, thanks! Just for completeness, I will as @rabdill to check the version reporting as well to complete his checklist.

rabdill commented 5 years ago

@csoneson All set! Sorry for the delay.

bii-absd commented 5 years ago

@csoneson, @andreysmelter and @rabdill - Thank you all!

csoneson commented 5 years ago

@whedon accept

whedon commented 5 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 5 years ago

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/880

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/880, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
csoneson commented 5 years ago

@openjournals/joss-eics - over to you!