search

openjournals / joss-reviews

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

[REVIEW]: PopMedNet: A scalable and extensible open-source informatics platform designed to facilitate the implementation and operation of distributed health data networks #4062

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@ddeehere<!--end-author-handle-- (Daniel Dee) Repository: https://github.com/PopMedNet-Team/popmednet Branch with paper.md (empty if default branch): Version: PMN-2021.6-JOSS Editor: !--editor-->@danielskatz<!--end-editor-- Reviewers: @jhancock4d, @lrasmus Archive: 10.5281/zenodo.6325732

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@jhancock4d & @lrasmus, 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 @danielskatz know.

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

Review checklist for @jhancock4d

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @lrasmus

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

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

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

: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

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

@whedon generate pdf
whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.13063/2327-9214.1213 is OK
- 10.1097/MLR.0b013e3181d9919f is OK
- 10.17294/2330-0698.1149 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 2 years ago

Wordcount for paper.md is 740

danielskatz commented 2 years ago

@jhancock4d and @lrasmus - Thanks for agreeing to review this submission. This is the review thread for the paper. All of our communications will happen here from now on.

Both reviewers have checklists at the top of this thread with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

Please read the first couple of comments in this issue carefully, so that you can accept the invitation from JOSS and be able to check items, and so that you don't get overwhelmed with notifications from other activities in JOSS.

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

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

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

danielskatz commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

whedon commented 2 years ago

:wave: @jhancock4d, please update us on how your review is going (this is an automated reminder).

whedon commented 2 years ago

:wave: @lrasmus, please update us on how your review is going (this is an automated reminder).

jhancock4d commented 2 years ago

I believe I'm complete. My only question is as to what constitutes sufficient testing. The unit testing is very light as it currently stands.

danielskatz commented 2 years ago

@jhancock4d - thanks!

Re testing, see https://joss.readthedocs.io/en/latest/review_criteria.html#tests Does this meet the "OK" there?

danielskatz commented 2 years ago

@jhancock4d - I note you didn't check off the license item - can you? Or is there a problem?

danielskatz commented 2 years ago

@lrasmus - It doesn't look like you've gotten started on your checklist. Are there any problems I can help with?

jhancock4d commented 2 years ago

@danielskatz Sorry. Done. Only thing I'm still on the fence about is the unit testing. I'd like to see more to be confident that it covers the requirements.

danielskatz commented 2 years ago

@jhancock4d - can you say a bit more? Is this something where you are unsure about what is needed, or where you know what's needed and are unsure if it's being met? Is https://joss.readthedocs.io/en/latest/review_criteria.html#tests helpful?

lrasmus commented 2 years ago

@danielskatz - I am sorry, this was me not being able to balance priorities w/ projects at work. Things have cleared up, will work on it now and get done ASAP. Very sorry for the delays.

danielskatz commented 2 years ago

@lrasmus - no worries - issues come up for all of us

lrasmus commented 2 years ago

Status update - unchecked items are pending responses from open issues in source repo.

lrasmus commented 2 years ago

@jhancock4d - I'm working with the authors via https://github.com/PopMedNet-Team/popmednet/issues/38 on setup instructions for running the unit tests. Curious if you had a CDM database you used for the tests, if you used SynPuf or something else? Am going to try to get this going on my own, and I know the authors are working on instructions.

jhancock4d commented 2 years ago

@jhancock4d - I'm working with the authors via PopMedNet-Team/popmednet#38 on setup instructions for running the unit tests. Curious if you had a CDM database you used for the tests, if you used SynPuf or something else? Am going to try to get this going on my own, and I know the authors are working on instructions.

I just ran the migrations which got me a blank database and worked from there to setup everything by hand. (I have an SQL DB server laying around but the developer edition works fine.)

The setup of the database probably needs better documentation. However, my main issue with testing is that there aren't very MANY tests, not that they don't work (they seem to)

lrasmus commented 2 years ago

@danielskatz - thank you for your ongoing patience. I am about ready to sign off. I have asked the authors to add a 'State of the Field' to the paper, and then I think this meets all requirements.

@jhancock4d - with your assessment of tests, were you able to run code coverage (I was not)? I was able to get 720 passed tests with the test suite they provided. They seem more like integration tests than unit tests, but would consider it 'OK' for the purposes of the evaluation. Curious if there were specific areas you thought needed more testing?

jhancock4d commented 2 years ago

@irasmus The integration tests are actually a feature not a bug in this case I would say because unit tests on a project that deals with data that would use mocks wouldn't be useful.

Code coverage is relatively lowish from what I can tell (I can't get coverlet to spit out data on it). I think it's fine for this purpose, but in general it would be nice to see 80%+ code coverage in the future with coverlet commands that would do code coverage reporting along with the testing.

danielskatz commented 2 years ago

👋 @ddeehere - I think we're just waiting for you to address https://github.com/PopMedNet-Team/popmednet/issues/39 at this point, and I see there that you are working on it. Please let us know when there's something for the reviewers to check.

ddeehere commented 2 years ago

@danielskatz @lrasmus I added a section "Comparison with i2b2 and SHRINE" for "State of the Field" in paper.md.

lrasmus commented 2 years ago

Thank you @ddeehere!

@danielskatz - I've signed off on all checklist items, I think this is in good shape and set to go.

danielskatz commented 2 years ago

👋 @jhancock4d - can you confirm that you are ready for this to be published?

danielskatz commented 2 years ago

@whedon check references

danielskatz commented 2 years ago

@whedon generate pdf

jhancock4d commented 2 years ago

@danielskatz Confirmed!

whedon commented 2 years ago

PDF failed to compile for issue #4062 with the following error: 

 Error reading bibliography file paper.bib:
(line 36, column 91):
unexpected "a"
expecting space, ",", white space or "}"
Looks like we failed to compile the PDF
danielskatz commented 2 years ago

@ddeehere - I think https://github.com/PopMedNet-Team/popmednet/pull/40 will fix the building issue, along with some formatting

danielskatz commented 2 years ago

@ddeehere - we're quite close to done; I hope you can accept this ☝️ PR soon, then I can proofread and move this to acceptance.

ddeehere commented 2 years ago

@ddeehere - we're quite close to done; I hope you can accept this ☝️ PR soon, then I can proofread and move this to acceptance.

@danielskatz Just merged the PR. Sorry failed to realize I have to do something. Thanks for reminder.

danielskatz commented 2 years ago

@editorialbot check references

danielskatz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

Attempting PDF compilation. Reticulating splines etc...

editorialbot commented 2 years ago

:warning: An error happened when generating the pdf.

danielskatz commented 2 years ago

@ddeehere - can you also merge https://github.com/PopMedNet-Team/popmednet/pull/41 - this was my error, sorry

danielskatz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

Attempting PDF compilation. Reticulating splines etc...

editorialbot commented 2 years ago

:warning: An error happened when generating the pdf.

danielskatz commented 2 years ago

@ddeehere - sorry, one more error - please merge https://github.com/PopMedNet-Team/popmednet/pull/42

ddeehere commented 2 years ago

@danielskatz Merged

danielskatz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

Attempting PDF compilation. Reticulating splines etc...

editorialbot commented 2 years ago

:warning: An error happened when generating the pdf.

danielskatz commented 2 years ago

@ddeehere - we're getting closer - this one (https://github.com/PopMedNet-Team/popmednet/pull/43) is one where the ref name in the paper doesn't match the name in the bib

ddeehere commented 2 years ago

@danielskatz Thanks for catching that. Merged.

danielskatz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

Attempting PDF compilation. Reticulating splines etc...

danielskatz commented 2 years ago

@editorialbot check references