[REVIEW]: PulseqDiffusion: PyPulseq implementation of Diffusion-Weighted Echo Planar Imaging #2478
Comments
|
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 🎉. 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. ⭐ Important ⭐ 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:
For a list of things I can do to help you, just type: For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type: |
|
|
@ritagnunes can you fix those DOI's ☝️, FYI you can run |
|
@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! |
|
@whedon generate pdf
…On Sun, 12 Jul 2020, 22:26 Kevin Mattheus Moerman, ***@***.***> 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
<#2478 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIZGNBBMG5HPVYFV62LA2Z3R3ITBVANCNFSM4OX33ETA>
.
|
I have not done a real review yet, but based on a quick look, the primary contribution is the three files named 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 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. |
|
@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. |
|
@whedon re-invite @mathieuboudreau as reviewer |
|
OK, the reviewer has been re-invited. @mathieuboudreau please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations |
|
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. |
|
@ritagnunes can you provide an update on progress in relation to responding to the issues raised? |
|
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 < ***@***.***> 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
<#2478 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIZGNBAHW6U3RU24B7SND6TR7EX6DANCNFSM4OX33ETA>
.
|
|
See comment ritagnunes/PulseqDiffusion#4 (comment) 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, 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. |
|
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 ***@***.***> wrote:
See comment ritagnunes/PulseqDiffusion#4 (comment)
<ritagnunes/PulseqDiffusion#4 (comment)>
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
<#2478 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIZGNBCEEVEV74HUNOIR6CTR7RGEFANCNFSM4OX33ETA>
.
|
|
@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. |
|
Thanks - I'm on it this afternoon. |
|
Thanks to some clarifications from the authors (ritagnunes/PulseqDiffusion#4 (comment), ritagnunes/PulseqDiffusion#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 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:
In particular, this line in the JOSS guidelines is where I have pause here:
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). |
|
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? |
Yes, I think those comments are accurate and was also uncertain that it met those the criteria. I agree with his summary:
The only difference I see from what @mathieuboudreau reported is that the I did also try running the unit tests today via <\details> |
|
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 ***@***.***> 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
<#2478 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIZGNBGJQUC554H3W43ES5LSCPBQPANCNFSM4OX33ETA>
.
|
|
@whedon check repository |
|
|
@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. |
|
@whedon reject |
|
Paper rejected. |
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
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 badge code:
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:
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
The text was updated successfully, but these errors were encountered: