search

openjournals / joss-reviews

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

[REVIEW]: PyQMRI: An accelerated Python based Quantitative MRI Python toolbox #2727

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @maieroli2010 (Oliver Maier) Repository: https://github.com/IMTtugraz/PyQMRI Version: v1.0.0 Editor: @Kevin-Mattheus-Moerman Reviewer: @grlee77, @agahkarakuzu, @DARSakthi Archive: 10.5281/zenodo.4313301

: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/4578e430042552e15b49d1d72789b72c"><img src="https://joss.theoj.org/papers/4578e430042552e15b49d1d72789b72c/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/4578e430042552e15b49d1d72789b72c/status.svg)](https://joss.theoj.org/papers/4578e430042552e15b49d1d72789b72c)

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

@grlee77 & @agahkarakuzu & @DARSakthi, 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 @Kevin-Mattheus-Moerman 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 @grlee77

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @agahkarakuzu

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @DARSakthi

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @grlee77, @ @agahkarakuzu, @DARSakthi 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 4 years ago

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

/app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in block in find': No such file or directory - e1e19e2b7e83c454b0cb0797 (Errno::ENOENT) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:incollect!' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-d14a699185fb/lib/whedon/processor.rb:66:infind_paper_paths' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-d14a699185fb/bin/whedon:53:in prepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-d14a699185fb/bin/whedon:131:in<top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in

'

Kevin-Mattheus-Moerman commented 4 years ago

@whedon generate pdf from branch joss-paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 4 years ago

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

sh: 1: cd: can't cd to 09e65fba2022502e74700760 /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in block in find': No such file or directory - 09e65fba2022502e74700760 (Errno::ENOENT) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:incollect!' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-d14a699185fb/lib/whedon/processor.rb:66:infind_paper_paths' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-d14a699185fb/bin/whedon:53:in prepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-d14a699185fb/bin/whedon:131:in<top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in

'

Kevin-Mattheus-Moerman commented 4 years ago

@whedon generate pdf from branch JOSS_pub

whedon commented 4 years ago 
Attempting PDF compilation from custom branch JOSS_pub. Reticulating splines etc...
whedon commented 4 years ago

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

pandoc-citeproc: reference agahkarakuzu not found Error producing PDF. ! TeX capacity exceeded, sorry [input stack size=5000]. \reserved@a ->\def \reserved@a *{\let \@xs@assign \@xs@expand@and@detokenize... l.336 }

Looks like we failed to compile the PDF

Kevin-Mattheus-Moerman commented 4 years ago

@MaierOli2010 can you check :point_up: that reference?

MaierOli2010 commented 4 years ago

@Kevin-Mattheus-Moerman agahkarakuzu should be the git name of one of the reviewers. I do not have any reference that could conatin that name though (just checked the paper.bib file).

Kevin-Mattheus-Moerman commented 4 years ago

@openjournals/dev @arfon have a look at this issue :point_up:, it looks like when I assigned agahkarakuzu as reviewer over at https://github.com/openjournals/joss-reviews/issues/2718 they got a double @ in their assigned name, see also at the top of this issue. Looks like that might be causing this. How will I fix this? Will I remove @ @agahkarakuzu, and add @agahkarakuzu as reviewer again?

arfon commented 4 years ago

@whedon generate pdf from branch JOSS_pub

whedon commented 4 years ago 
Attempting PDF compilation from custom branch JOSS_pub. Reticulating splines etc...
arfon commented 4 years ago

@Kevin-Mattheus-Moerman - I just edited the issue manually.

whedon commented 4 years ago

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

agahkarakuzu commented 4 years ago

@Kevin-Mattheus-Moerman I came back to this issue to see the latest changes, then noticed that I still don't appear on the list of reviewers.

Kevin-Mattheus-Moerman commented 4 years ago

@agahkarakuzu I think this looks good now right? Can you confirm you are able to tick boxes?

Kevin-Mattheus-Moerman commented 4 years ago

@grlee77, @agahkarakuzu, @DARSakthi thanks for your review efforts!!!

If see some of you have left some boxes unticked. If you could summarize for the authors what work is needed that would be great.

Thanks.

agahkarakuzu commented 4 years ago

@Kevin-Mattheus-Moerman I cannot check the boxes :/

arfon commented 4 years ago

@whedon re-invite @agahkarakuzu as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

@agahkarakuzu please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

arfon commented 4 years ago

@agahkarakuzu - try again now (once you've clicked and accepted the invite)

agahkarakuzu commented 4 years ago

Thank you @arfon now I have edit access!

MaierOli2010 commented 4 years ago

I've added basic Contributing guidelines to the project as well as a "Statement of Need" in the README.rst. Further, I've made a very basic "How to use" Guide on Google Colab which might needs more formatting but gives a potential use a quick insight into how to use PyQMRI.

DARSakthi commented 4 years ago

Hi everyone,

Apologies for the delay in responding to this review. October and November are very busy months -- some NIH deadlines to prepare for.

A few comments --

Firstly, the scholarly effort is substantial. I commend the authors on not only the code but the methods development.

I noticed you mention you added some google colab resources, but I can't seem to find them. Could you point me in their direction? Because google colab offers limited but free online GPU usage, I think it would be a good idea for you to move towards integrating it within your package. Perhaps as a separate release. This isn't crucial to publication -- merely a suggestion.

The documentation is sufficiently complete.

I recommend, prior to publication, some copy editing on the submitted manuscript. I can provide specific examples of sentences which I think could be better phrased later.

Otherwise I think this is a good paper. Once the above suggestion about language is addressed I would recommend acceptance for publication.

Cheers

Dalton Sakthivadivel

Kevin-Mattheus-Moerman commented 4 years ago

@DARSakthi thanks for your comments.

@MaierOli2010 can you work on this :point_up:

MaierOli2010 commented 4 years ago

Thank you for the comments!

The links is in the "Running the reconstruction" section, last sentence. It is not very prominently placed. You can also use this link to have a look.

If you could mark the sentences that should be rephrased that would be great.

Cheers

Oliver

Kevin-Mattheus-Moerman commented 4 years ago

@DARSakthi Can you help @MaierOli2010 to point them in the right direction? :point_up:, thanks!

grlee77 commented 4 years ago

I am also overall in favor of this publication and will update with any final comments later today.

Great job on the collab notebook. That will be very helpful to potential users.

agahkarakuzu commented 4 years ago

@MaierOli2010 thank you sou much for providing the colab notebook! I was having some issues with the CUDA version I have on my local machine, this addition dispensed the need for dealing with it. Before I begin, I would like to give full disclosure that I am the author of qMRLab, a software for quantitative MRI analysis.

This submission effectively fulfills the scope eligibility, providing a powerful framework to ease the processing of 3D model-based reconstruction. I am happy to see the methods from this work will be available as a Python package that meets JOSS standards. Besides the convenience PyQMRI brings to qMRI analysis with under-sampled data, the idea of tapping into symbolic expressions to introduce new models is a great opportunity for users to easily expand this toolbox according to their needs.

To be able to check all the boxes, I have some comments regarding the documentation and the article:

Documentation

1) The sphinx auto-generated API documentation is of good service for users accustomed to the hierarchy of Python modules and for developers who’d like to contribute. Despite that it is delivered by a combination of README.md and the colab notebook, a nicely organized beginner’s guideline at the RTD page would make PyQMRI more user friendly.

The TOC in the colab notebook is intuitive, the example data is publicly available, and the basic functionality can be tested without worrying about the availability/compatibility of GPU resources. Centralizing the documentation by organizing these pieces together would lead to a more informative guideline and reduce the risk of users missing out on offered functionality. This way, README.md would become leaner, conveying the high-level functionality of the toolbox without overloading new users with usage details or the requisites about the input data structure.

2) Travis build checks the proper installation of the software and the minimum functionality. I see that the unit tests are performed by Jenkins given the need for GPU. Is it possible to show the state of the Jenkins build on GitHub, ideally with the build logs? I know such integrations are not provided out of the box by Jenkins, but if there is a plugin that can enable this through webhooks, it would do the finishing touch on the CI picture of the repository. If this is not possible, adding a small note about how these tests are performed in the README can be useful.

3) 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

I was not able to locate the community guideline. Can you please point me to it if exists, or add it otherwise?

Software paper

Overall, the article is well-written and clearly states which gap does the PyQMRI address.

State of the field: Do the authors describe how this software compares to other commonly-used packages?

However, I was not able to check this box as such information is currently missing in the article. For the sake of completeness, it is worth mentioning other open-source qMRI toolboxes (e.g. hMRI Toolbox, QUIT, qMRLab, QMAP, mrQ, to name a few) and where does PyQMRI stand in relation to them. I would like to note that two of these toolboxes are published in JOSS and I am looking forward to seeing PyQMRI added to that list.

Once these comments are addressed, I am in favor of this publication as well.

Best regards!

MaierOli2010 commented 4 years ago

@agahkarakuzu thank you for the detailed list of comments and also for pointing me into the right direction, showing a multitude of qMRI toolboxes that are currently available. Unfortunately, I am quite busy this week and will be able to have a detailed look at them early next week. At the first glance the major difference, beside offering different types of models, seems to be that these expect image data as input and thus require image reconstruction algorithms prior to their usage (if undersample data is acquired of course). Some of them integrate with registration toolboxes (e.g ITK, SPM) to offer registration prior to fitting. I will have a closer look as soon as possible and update the paper with a corresponding section.

I will add the missing community guidlines as soon as possbile.

I am not sure about your point about the Jenkins build. The CI currently runs on a machine, nested in the university network. A quick search did not give any informaiton on how to upload build logs with webhooks to GitHub, only notifications about success/failure of test cases, which is already integrated in the Github repo. Might need to dig deeper into that but it might also not be possible at all without exposing the Jenkins host to the web, i.e. publicly hosting the jenkins webserver (which is an option I was thinking about in the past but it might also not be well received by our admin).

I do agree with the points about the documentation and will streamline and integrate the Google Colab parts to it. The parts about prerequest and specific requirements not covered by the notebook will move into the sphinx documentation and a link will be added to the README.MD.

Thank you again for your comments.

Best regards!

agahkarakuzu commented 4 years ago

Thank you for the quick response @MaierOli2010, sure whenever you are available.

which is already integrated in the Github repo

Sorry I was not clear, I meant the build status, like the one belongs to your Travis CI builds. At any rate, this is not a major issue, it would be nice to have public access to that information. But if the Jenkins instance's being installed on a campus server is going to make it tricky and/or if this is not an easy task, feel free to skip this one.

grlee77 commented 4 years ago

I was not able to locate the community guideline. Can you please point me to it if exists, or add it otherwise?

I considered the CONTRIBUTING.rst file to fill this role. It doesn't cover things like expected code of conduct/behavior, but I don't think that is necessarily a requirement? If interested, feel free to adapt a basic, informal set of community guidelines like this one from scikit-image.

grlee77 commented 4 years ago

The API documentation seems fine and I think the new collab notebook serves as a reasonable beginner-friendly introduction to usage of the software so I have now checked off these items in the checklist.

I agree with the point related to a need for a brief summary of the state of the field (and corresponding references) which is why these are the only remaining items to address for me. I agree with @MaierOli2010 that this package is fairly unique in that reconstruction and quantification are combined in a single optimization problem from the raw k-space data. I think BART has something similar for non-linear inverse reconstruction for T1 mapping, but that is the only one I can recall offhand. I think the model in BART is hard-coded, though, so the user cannot easily modify it.

DARSakthi commented 4 years ago

Hello,

The colab notebook works wonderfully. I will state again that I think it will attract a wider user base -- even in this era of high powered computing, not everyone has consistent access to a GPU.

As far as language goes, for example, the very first sentence is a bit messy. I would try and make it flow better. I recommend a copy editing software like Grammarly or the like. I have marked some portions of it here with some possible revisions. Do let me know if you can't access, or would like me to transcribe the comments as a reply in this thread. Overall they are minor suggestions, and only suggestions -- you must write your paper the way you see fit. The algorithmic section was notably well written.

Regarding other reviewer comments, I found the description of the 'state of the field' sufficiently discussed the current standards in qMR -- but I do agree with suggestions for giving specific examples of similar packages.

DARSakthi commented 4 years ago

By the way -- Microsoft word has a pretty nice grammar and spelling processor these days. This could provide some guidance. Especially if you don't want to spend the money on a grammarly license, as I believe it has a cost associated. Paper is attached in case of access issues. qmri_joss.pdf

MaierOli2010 commented 4 years ago

Thank you for you comments and the time you spent to correct the grammar issues. I will update the paper accordingly.

Kevin-Mattheus-Moerman commented 3 years ago

@MaierOli2010 let us know when you've made changes to review. FYI you can also call @whedon generate pdf here to update the paper.

MaierOli2010 commented 3 years ago

@Kevin-Mattheus-Moerman I've updated the paper based on the suggestions of the reviewers.

I am still trying to somehow make an better integration with Jenkins. Everything else should be addressed.

MaierOli2010 commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

Can't find any papers to compile :-(

MaierOli2010 commented 3 years ago

@whedon generate pdf from branch JOSS_pub

whedon commented 3 years ago 
Attempting PDF compilation from custom branch JOSS_pub. Reticulating splines etc...
whedon commented 3 years ago

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

MaierOli2010 commented 3 years ago

@agahkarakuzu I've finally managed to get a working integration with Jenkins through webhooks. There are just two downsides to this. First, as soon as I have to restart the web forwarding the hook changes and the README needs to be updated with the new links. Second, as the links are static in the README they are currently pointing to master only. Other than that it is possible to view the Jenkins build and all the reports now by clicking on the banner. One small thing that is currently not linking correctly is the "details" link if clicking on the green/red mark next to the commit which still points to localhost.

agahkarakuzu commented 3 years ago

@MaierOli2010 it looks great! Thank you so much for addressing this issue, I think master only serves its purpose well.

grlee77 commented 3 years ago

Thanks @MaierOli2010, the new text gives a good overview of related tools. I have checked off the remaining items under my review above, but the new text does need some additional proofreading/typo fixes:

specific typos:

reference formatting issues:

MaierOli2010 commented 3 years ago

@grlee77 Please excuse the typos. I do not know how they managed to slip through again.. The manuscript should be updated. I've also done some rephrasing in the new section to avoid too many repetitions of "tool"

MaierOli2010 commented 3 years ago

@whedon generate pdf from branch JOSS_pub

whedon commented 3 years ago 
Attempting PDF compilation from custom branch JOSS_pub. Reticulating splines etc...