search

openjournals / joss-reviews

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

[REVIEW]: PulseqDiffusion: PyPulseq implementation of Diffusion-Weighted Echo Planar Imaging #2478

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @ritagnunes (Rita G. Nunes) Repository: https://github.com/ritagnunes/PulseqDiffusion Version: v1.0.0 Editor: @Kevin-Mattheus-Moerman Reviewer: @mathieuboudreau, @grlee77 Archive: Pending

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

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

@mathieuboudreau & @grlee77, 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 try and complete your review in the next six weeks ✨

Review checklist for @mathieuboudreau

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @grlee77

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. @mathieuboudreau, @grlee77 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 
Reference check summary:

OK DOIs

- 10.3389/fninf.2014.00008 is OK
- 10.1002/jmri.26638 is OK
- 10.1002/mrm.26235 is OK
- 10.1002/jmri.26637 is OK
- 10.1002/mrm.26990 is OK
- 10.1016/j.mri.2018.03.008 is OK
- 10.21105/joss.01725 is OK
- 10.1002/mrm.10308 is OK
- 10.1212/WNL.0b013e3181e7c9dd is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/j.neuroimage.2011.09.015 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1016/j.neuroimage.2019.116137 is INVALID because of 'https://doi.org/' prefix
whedon commented 4 years ago

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

Kevin-Mattheus-Moerman commented 4 years ago

@ritagnunes can you fix those DOI's :point_up:, FYI you can run @whedon generate pdf here to update the paper, or run @whedon check references to check those DOI's. Thanks

Kevin-Mattheus-Moerman commented 4 years ago

@mathieuboudreau, @grlee77 thanks for your help here! I look forward to seeing your review comments. I would like to point out that judging from the number of lines of code and the content of the repo this submission could be on the skinny side of what is typically accepted in JOSS. If you feel this submission is too minor (in terms of functionality, research application, etc) please do flag this with me. Thanks!

ritagnunes commented 4 years ago

@whedon generate pdf

On Sun, 12 Jul 2020, 22:26 Kevin Mattheus Moerman, notifications@github.com wrote:

@mathieuboudreau https://github.com/mathieuboudreau, @grlee77 https://github.com/grlee77 thanks for your help here! I look forward to seeing your review comments. I would like to point out that judging from the number of lines of code and the content of the repo this submission could be on the skinny side of what is typically accepted in JOSS. If you feel this submission is too minor (in terms of functionality, research application, etc) please do flag this with me. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2478#issuecomment-657277655, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIZGNBBMG5HPVYFV62LA2Z3R3ITBVANCNFSM4OX33ETA .

whedon commented 4 years ago

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

grlee77 commented 4 years ago

@mathieuboudreau, @grlee77 thanks for your help here! I look forward to seeing your review comments. I would like to point out that judging from the number of lines of code and the content of the repo this submission could be on the skinny side of what is typically accepted in JOSS. If you feel this submission is too minor (in terms of functionality, research application, etc) please do flag this with me. Thanks!

I have not done a real review yet, but based on a quick look, the primary contribution is the three files named write_*.py in this folder. These files generate three different variants of a vendor-neutral diffusion MRI sequence using the PyPulseq tool I previously helped review for JOSS. There was already a basic EPI example in PyPulseq itself, so the new part is the addition of the diffusion-encoding gradients and either one or two 180 degree refocusing pulses. These turn the pulse sequence from a basic gradient-echo EPI sequence into spin-echo one that is sensitive to diffusion. This type of sequence is widely used in diffusion-based neuroimaging studies.

I am sympathetic to the goal of providing a vendor-neutral implementation of these sequences and am not aware of another existing source for such sequences. In practice, research sites that have a commercial license for a vendor-specific implementation are going to continue to use that for real-world studies, as they are more widely tested and often incorporate additional features (simultaneous multi-slice, parallel imaging, etc.). There is still value in having open source implementations as a teaching tool, at sites without an equivalent commercial sequence, and potentially for some more specialized research cases.

Aside from the pulse sequence generating functions, the authors provide the file recon_imgs.m which the user would use to make images from the k-space data acquired by these sequences. The provided reconstruction script looks like it would work for two of the variants, but I'm not sure about the "ramp-sampling" variant. I did not see any code to regrid the non-uniformly spaced samples (due to sampling on the ramps) back to a uniformly spaced grid, but it is possible I missed it somewhere.

The recon script included is pretty basic compared to more general open source MRI reconstruction tools, but it does match the provided pulse sequence and should simplify reconstruction for end users that may not be image reconstruction specialists. Therefore, it does add value above just providing the sequence alone.

In summary, I think the submission has value, but agree that it is a somewhat incremental addition on top of PyPulseq itself.

mathieuboudreau commented 4 years ago

@Kevin-Mattheus-Moerman the "whedon invited you to openjournals/joss-reviews" email link has expired, since I was on vacation last week. Could someone trigger a re-send? Thanks.

Kevin-Mattheus-Moerman commented 4 years ago

@whedon re-invite @mathieuboudreau as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

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

mathieuboudreau commented 4 years ago

I've started to review this submission, and opened several 6 issues in their repositories (issues 1 through 6 in https://github.com/ritagnunes/PulseqDiffusion/issues). I can't proceed until some of those are resolved, as the installation/example usage/funcitonality/tests are incomplete or not well documented (i.e. I can't figure out how to run/test the software yet).

In addition, @ritagnunes could you please list why the authors are listed as such, and what contribution they had on this project. I could identify several as contributors in the repository, but some I couldn't.

Kevin-Mattheus-Moerman commented 4 years ago

@ritagnunes can you provide an update on progress in relation to responding to the issues raised?

ritagnunes commented 4 years ago

We are working towards solving these issues asap.

Regarding the contribution of each author, here is the answer to @mathieuboudreau https://github.com/mathieuboudreau 's question:

@ritagnunes - joint implementation of write*py sequence files, implementation and tests for some of the auxiliary diffusion functions, implementation and testing of basic reconstruction functions, joint project supervision and manuscript preparation @sravan953 - performing MRI experiments for testing the generated sequences and generating example data, providing basic documentation and testing code @opennog - joint implementation of write*.py sequence files, implementation and tests for some of the auxiliary diffusion functions @ZemaTimoteo - implementation of the SNR study code, its auxiliary functions and corresponding tests @mvgolub - theoretical derivation of b-value optimization for SNR study, review and testing of the SNR study code @PavanPoojar - provided basic documentation and testing code @sairamgeethanath - provided basic documentation, joint project supervision and manuscript preparation

On Wed, Aug 5, 2020 at 11:28 AM Kevin Mattheus Moerman < notifications@github.com> wrote:

@ritagnunes https://github.com/ritagnunes can you provide an update on progress in relation to responding to the issues raised?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2478#issuecomment-669115196, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIZGNBAHW6U3RU24B7SND6TR7EX6DANCNFSM4OX33ETA .

grlee77 commented 4 years ago

See comment https://github.com/ritagnunes/PulseqDiffusion/issues/4#issuecomment-670660507 regarding how this package is a mixture of python and Matlab scripts and not a traditional, installable software library . Given, that, why did the team decide to use the Python-based pypulseq, but keep the other scripts in Matlab?

For example, recon_img.m looks like it would be easy to convert from Matlab to Python using just functions in the NumPy library (fft, fftshift, squeeze, etc.). If the authors prefer Matlab, then why use pypulseq rather than the original Matlab-based pulseq for the pulse sequence generation scripts? I personally prefer Python, but realize Matlab is also very popular in MR research labs. It just seems a little odd to mix the two rather than choosing one or the other.

Also, it looks like these Matlab scripts don't rely on specific toolboxes and probably just work in Octave? If that is the case, I would mention that so that Python users who might be interested will know that they don't necessarily need a commercial Matlab license to run the scripts.

ritagnunes commented 4 years ago

Dear @mathieuboudreau https://github.com/mathieuboudreau, @grlee77 https://github.com/grlee77 ,

Many thanks for your constructive feedback. We have made some modifications to the code to address the issues you raised. I have also added a comment regarding the mixed use of Matlab and Python.

Let me know if you need any further information to proceed with the revision.

On Fri, Aug 7, 2020 at 8:06 PM Gregory R. Lee notifications@github.com wrote:

See comment ritagnunes/PulseqDiffusion#4 (comment) https://github.com/ritagnunes/PulseqDiffusion/issues/4#issuecomment-670660507 regarding how this package is a mixture of python and Matlab scripts and not a traditional, installable software library . Given, that, why did the team decide to use the Python-based pypulseq, but keep the other scripts in Matlab?

For example, recon_img.m looks like it would be easy to convert from Matlab to Python using just functions in the NumPy library (fft, fftshift, squeeze, etc.). If the authors prefer Matlab, then why use pypulseq rather than the original Matlab-based pulseq https://pulseq.github.io/matlab.html for the pulse sequence generation scripts? I personally prefer Python, but realize Matlab is also very popular in MR research labs. It just seems a little odd to mix the two rather than choosing one or the other.

Also, it looks like these Matlab scripts don't rely on specific toolboxes and probably just work in Octave? If that is the case, I would mention that so that Python users who might be interested will know that they don't necessarily need a commercial Matlab license to run the scripts.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2478#issuecomment-670667843, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIZGNBCEEVEV74HUNOIR6CTR7RGEFANCNFSM4OX33ETA .

Kevin-Mattheus-Moerman commented 4 years ago

@mathieuboudreau, @grlee77 if you attended ISMRM I hope you enjoyed it. This is just a friendly check in to see where we stand with this submission. Let me know if you have any questions.

mathieuboudreau commented 4 years ago

Thanks - I'm on it this afternoon.

mathieuboudreau commented 4 years ago

Thanks to some clarifications from the authors (https://github.com/ritagnunes/PulseqDiffusion/issues/4#issuecomment-669341558, https://github.com/ritagnunes/PulseqDiffusion/issues/6), I’ve been able to better understand the authors goal for the code and test it out a bit more.

After testing the code a bit, to my understanding (and as @grlee77 pointed out) this submission is more of a collection of scripts/functions than a piece of software. There are two main problems that this code tries to be a solution to from my understanding : #1 create a diffusion pulse sequence (e.g. list of events to be executed by the MRI hardware) with the PyPulseq tool (authors provide three demos), and #2 take raw data that’s acquired from the MRI hardware using the above pulse sequence(s) and reconstruct images that can be used in analyses.

The authors implementation for #1 are just Python scripts (wrapped in a __main__ function) where all the relevant values are hardcoded (i.e. can't be passed to the function), and the user is asked without real guidance to modify or update them inside the scripts for their own particular application. The scripts depend entirely on another piece of software (PyPulseq), and in themselves could have been just as easily integrated as addons to that software instead of this proposed standalone PulseqDiffusion. I checked, and there is also substantial duplicate code between two of the three scripts offered by the authors (write_se_dwi_epi_bValue.py and write_se_dwi_rsepi_bValue.py), thus these scripts could in fact be refactored into modular functions that could then be tested and a single API could be offered to the user. Also, hardcoding parameters that need to be changed by the user inside the files increases the level of difficulty in terms of user experience (providing a way to pass along these parameters as a file would be better in my view, as then these files could be shared or kept track of).

The authors implementation for #2 are MATLAB files that are completely separate from the implementation for #1. I tried running their example again today, but was unable to run it as it appears that my fresh installation of FSL (a dependency) doesn’t have a file called by the authors (missing eddy_openmp, though my installation has eddy eddy_correct eddy_quad eddy_combine eddy_cuda7.5 eddy_squada). It may just be that the authors have an older version of FSL installed, although no specific version for any of their dependencies are stated in their installation instructions.

I think overall, due to the presence of a lack of coherence in the code (no link between parts 1 and 2 above in terms of code or user experience) and strong dependency on other softwares (part 1 is essentially scripts implementing PyPulseq use cases, but isn’t a piece of software in itself), I’m really on the fence about if this merits a tick on the JOSS checkbox:

Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

In particular, this line in the JOSS guidelines is where I have pause here:

In addition, JOSS requires that software should be feature-complete (i.e. no half-baked solutions) and designed for maintainable extension (not one-off modifications of existing tools). “Minor utility” packages, including “thin” API clients, and single-function packages are not acceptable.

As with @grlee77, I think this work has value and that substantial work has been done by the authors, and by looking at the code I don’t have doubts on the quality or MR physics of their implementation. But in my view, I don’t think in its current state it represents a coherent piece of software (i.e. "feature complete"?), but rather a collection of scripts & tools ("minor utility" package?). Does this warrant a publication in JOSS? As a somewhat newbie JOSS reviewer, I’m leaning towards no in its current state, but think with an improved user experience and more coherence between all the parts of the code, it could be done (but would require significantly more work). My mind could be changed though if the JOSS editor (@Kevin-Mattheus-Moerman) chimes in and says that what I described above is within the scope of what JOSS wants from their submissions, if so then I’d have additional comments for suggestions to improve the software (since I still haven’t been able to test it all yet without issues).

Kevin-Mattheus-Moerman commented 4 years ago

Thank you @mathieuboudreau for highlighting these important points.

@grlee77 it would be great if you could also weigh in on these comments. Based on @mathieuboudreau 's comments this work does not warrant publication in JOSS (given https://joss.readthedocs.io/en/latest/submitting.html#substantial-scholarly-effort). Do you agree with his statements?

grlee77 commented 4 years ago

Do you agree with his statements?

Yes, I think those comments are accurate and was also uncertain that it met those the criteria. I agree with his summary:

But in my view, I don’t think in its current state it represents a coherent piece of software (i.e. "feature complete"?), but rather a collection of scripts & tools ("minor utility" package?).

The only difference I see from what @mathieuboudreau reported is that the process_data.m example did work for me (FSL 5.0.11 on my system had the required eddy_openmp binary).

I did also try running the unit tests today via runTests_ReconAndProcessing.m. Some passed, but others failed with complaints about Matlab's norm function expecting 2D data. I don't know if this is an issue of a difference in Matlab version (I tried with R2017a) or if the authors had a custom n-dimensional norm.m implementation that is missing from the repo. Details are reproduced below.

``` Error occurred in recon_imgsTest/TestExampleInVivoData12Directions_Nb1_20Slices_1 and it did not run to completion. --------- Error ID: --------- 'MATLAB:norm:inputMustBe2D' -------------- Error Details: -------------- Error using norm Input must be 2-D. Error in assertWithAbsTol (line 6) tf = norm(actVal-expVal) <= tol; Error in recon_imgsTest (line 20) assertWithAbsTol(imact,impred,'Reconstructed images do not match predictions: Phantom3dirs5b3slices') ================================================================================ . Done recon_imgsTest __________ Failure Summary: Name Failed Incomplete Reason(s) ================================================================================================ estimate_adcTest/TestExampleInVivoData3Directions_Nb3_20Slices X X Errored. ------------------------------------------------------------------------------------------------ estimate_adcTest/TestExamplePhantomData3Directions_Nb5_3Slices X X Errored. ------------------------------------------------------------------------------------------------ recon_imgsTest/TestExampleInVivoData12Directions_Nb1_20Slices X X Errored. ------------------------------------------------------------------------------------------------ recon_imgsTest/TestExampleInVivoData3Directions_Nb3_20Slices X X Errored. ------------------------------------------------------------------------------------------------ recon_imgsTest/TestExampleInVivoData12Directions_Nb1_20Slices_1 X X Errored. testResults = 1×15 TestResult array with properties: Name Passed Failed Incomplete Duration Details Totals: 10 Passed, 5 Failed, 5 Incomplete. 17.0859 seconds testing time. ``` <\details>
ritagnunes commented 4 years ago

Hi @grlee77, I believe that is a Matlab version issue. The behaviour of the norm function must have changed. We have modified the code to use our own implementation. Let me know if you still have problems.

Thanks

On Tue, Aug 25, 2020 at 3:15 PM Gregory R. Lee notifications@github.com wrote:

Do you agree with his statements?

Yes, I think those comments are accurate and was also uncertain that it met those the criteria. I agree with his summary:

But in my view, I don’t think in its current state it represents a coherent piece of software (i.e. "feature complete"?), but rather a collection of scripts & tools ("minor utility" package?).

The only difference I see from what @mathieuboudreau https://github.com/mathieuboudreau reported is that the process_data.m example did work for me (FSL 5.0.11 on my system had the required eddy_openmp binary).

I did also try running the unit tests today via runTests_ReconAndProcessing.m. Some passed, but others failed with complaints about Matlab's norm function expecting 2D data. I don't know if this is an issue of a difference in Matlab version (I tried with R2017a) or if the authors had a custom n-dimensional norm.m implementation that is missing from the repo. Details are reproduced below.

Error occurred in recon_imgsTest/TestExampleInVivoData12Directions_Nb1_20Slices_1 and it did not run to completion.

---------

Error ID:

---------

'MATLAB:norm:inputMustBe2D'

--------------

Error Details:

--------------

Error using norm

Input must be 2-D.

Error in assertWithAbsTol (line 6)

tf = norm(actVal-expVal) <= tol;

Error in recon_imgsTest (line 20)

assertWithAbsTol(imact,impred,'Reconstructed images do not match predictions: Phantom3dirs5b3slices')

================================================================================

.

Done recon_imgsTest

Failure Summary:

 Name                                                             Failed  Incomplete  Reason(s)

================================================================================================

 estimate_adcTest/TestExampleInVivoData3Directions_Nb3_20Slices     X         X       Errored.

------------------------------------------------------------------------------------------------

 estimate_adcTest/TestExamplePhantomData3Directions_Nb5_3Slices     X         X       Errored.

------------------------------------------------------------------------------------------------

 recon_imgsTest/TestExampleInVivoData12Directions_Nb1_20Slices      X         X       Errored.

------------------------------------------------------------------------------------------------

 recon_imgsTest/TestExampleInVivoData3Directions_Nb3_20Slices       X         X       Errored.

------------------------------------------------------------------------------------------------

 recon_imgsTest/TestExampleInVivoData12Directions_Nb1_20Slices_1    X         X       Errored.

testResults =

1×15 TestResult array with properties:

Name

Passed

Failed

Incomplete

Duration

Details

Totals:

10 Passed, 5 Failed, 5 Incomplete.

17.0859 seconds testing time.

<\details>

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2478#issuecomment-680052051, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIZGNBGJQUC554H3W43ES5LSCPBQPANCNFSM4OX33ETA .

Kevin-Mattheus-Moerman commented 4 years ago

@whedon check repository

whedon commented 4 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.84  T=0.17 s (308.3 files/s, 30316.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
MATLAB                          26            267            395           1428
Python                          11            410            439           1357
Markdown                         6             80              0            237
TeX                              1             14              0            139
reStructuredText                 5             57             75             62
DOS Batch                        1              8              1             26
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            51            840            917           3258
-------------------------------------------------------------------------------

Statistical information for the repository '2478' was gathered on 2020/08/26.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Keerthi Sravan Ravi              4           478            368            7.26
PavanPoojar                      1           647              0            5.55
Rita G. Nunes                   17          3246           2821           52.08
opennog                         12          2636           1454           35.11

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Keerthi Sravan Ravi         352           73.6          0.1                0.00
Rita G. Nunes               794           24.5          1.0               17.51
opennog                    1060           40.2          1.2               17.74
Kevin-Mattheus-Moerman commented 4 years ago

@ritagnunes both reviewers have indicated this work is out of scope for JOSS. In particular it is deemed insufficiently complete and is classed as a "minor utility" tool as it stands (see our guidelines). This does not mean the software is not useful or of a low quality, it merely means that in terms of required structure and amount of scholarly content this work is out of scope for JOSS.

I will reject this submission at this point. If you continue to develop this work (expanding it, developing it further as a feature complete and independent package), we would be happy to reconsider it as a submission in the future. Alternatively you could archive a copy of this work on a service like ZENODO to obtain a DOI which users can cite.

Kevin-Mattheus-Moerman commented 4 years ago

@whedon reject

whedon commented 4 years ago

Paper rejected.