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]: GIRFReco.jl: An open-source pipeline for spiral MRI reconstruction in Julia #5877

Closed editorialbot closed 6 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@alexjaffray<!--end-author-handle-- (Alexander Jaffray) Repository: https://github.com/BRAIN-TO/GIRFReco.jl Branch with paper.md (empty if default branch): Version: v0.1.7 Editor: !--editor-->@Kevin-Mattheus-Moerman<!--end-editor-- Reviewers: @cncastillo, @mathieuboudreau, @felixhorger Archive: 10.5281/zenodo.11075548

Status

status

Status badge code:

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

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

@cncastillo & @mathieuboudreau & @felixhorger, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

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 ✨

Checklists

📝 Checklist for @cncastillo

📝 Checklist for @felixhorger

📝 Checklist for @mathieuboudreau

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.03 s (1073.7 files/s, 176673.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           24           1027            915           1941
XSLT                             1             58             30            840
TeX                              1             34              0            473
Markdown                         4             55              0            158
TOML                             1              3              0             96
Bourne Shell                     1              6              1             52
YAML                             3              9             12             49
-------------------------------------------------------------------------------
SUM:                            35           1192            958           3609
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 2231

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1002/mrm.23217 is OK
- 10.1002/mrm.24263 is OK
- 10.1002/mrm.28554 is OK
- 10.1002/jmri.20320 is OK
- 10.1109/TMI.2002.808360 is OK
- 10.1002/mrm.22767 is OK
- 10.1002/mrm.25827 is OK
- 10.1002/mrm.1241 is OK
- 10.1137/141000671 is OK
- 10.1002/mrm.26089 is OK
- 10.1002/mrm.25841 is OK
- 10.1002/mrm.24389 is OK
- 10.5281/ZENODO.592960 is OK
- 10.1002/mrm.29384 is OK
- 10.1002/mrm.28792 is OK
- 10.1002/mrm.24751 is OK
- 10.1109/TMI.2008.2006526 is OK
- 10.1109/TMI.2008.923956 is OK
- 10.1016/j.mri.2020.06.005 is OK
- 10.1002/mrm.27583 is OK
- 10.1109/TCI.2020.3031082 is OK
- 10.1002/mrm.1910390218 is OK
- 10.5281/zenodo.6510021 is OK
- 10.1002/mrm.27176 is OK
- 10.1016/j.neuroimage.2021.118674 is OK
- 10.1016/j.neuroimage.2017.07.062 is OK
- 10.1016/j.neuroimage.2021.118738 is OK

MISSING DOIs

- 10.58530/2022/2435 may be a valid DOI for title: Open-source model-based reconstruction in Julia: A pipeline for spiral diffusion imaging
- 10.58530/2022/0641 may be a valid DOI for title: MR System Stability and Quality Control using Gradient Impulse Response Functions (GIRF)

INVALID DOIs

- None
cncastillo commented 1 year ago

Review checklist for @cncastillo

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

cncastillo commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

felixhorger commented 1 year ago

Review checklist for @felixhorger

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

felixhorger commented 1 year ago

The DOIs it found for the ISMRM abstracts actually work, amazing, I didn't know about this 👍 @alexjaffray I think it's worth adding them into the .bib

felixhorger commented 1 year ago

Please find my comments on the manuscript here: 10.21105.joss.05877_FH.pdf.

The main point I'd like to bring up is that the paper makes it seem like there isn't a clear separation in the code of the (abstract) pipeline itself, reading and writing of data and additional features like plotting. In my opinion there should be a clear separation, outsourcing as much as possible (I'm a fan of an ecosystem of packages rather than a toolbox). This separation would make use more flexible, and adaptation to new scenarios easier (you also mention this point in your paper). How exactly this could look like I'll have to clarify after I read the code in more detail.

Kevin-Mattheus-Moerman commented 1 year ago

@alexjaffray could you please respond to the points raised by @felixhorger ?

Kevin-Mattheus-Moerman commented 1 year ago

@cncastillo, could you provide an update on review progress? Are there any key issues the authors should work on?

@mathieuboudreau, please can you start your review at this point? You can call @editorialbot generate my checklist to obtain your reviewer checklist.

mathieuboudreau commented 1 year ago

Review checklist for @mathieuboudreau

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

alexjaffray commented 1 year ago

Please find my comments on the manuscript here: 10.21105.joss.05877_FH.pdf.

The main point I'd like to bring up is that the paper makes it seem like there isn't a clear separation in the code of the (abstract) pipeline itself, reading and writing of data and additional features like plotting. In my opinion there should be a clear separation, outsourcing as much as possible (I'm a fan of an ecosystem of packages rather than a toolbox). This separation would make use more flexible, and adaptation to new scenarios easier (you also mention this point in your paper). How exactly this could look like I'll have to clarify after I read the code in more detail.

Thanks for the feedback! Your pdf comments seem to evolve over the course of the document, and you mentioned that you will read the code and clarify some things. Should we start to address your comments, or should we wait until you are finished (preferred)? We are also very happy to address each code review issue dynamically in Github, but likely it would be good to have all reviewers comments and suggestions first so that we don't duplicate work. Would this be alright with you @felixhorger

felixhorger commented 1 year ago

@alexjaffray yes I think that's a good idea, there are some minor comments on clarity which could be implemented already so that the other reviewers can proof-read that. The comments on the structure of the code can be left aside for now until all reviewers finished reading the code and gave their opinion.

Kevin-Mattheus-Moerman commented 1 year ago

@alexjaffray @felixhorger you may choose the approach that you like. However, usually we see the review progresses fastest/smoothest when the authors respond to issues directly as soon as they come in. This way the reviewers do not have to wait (their issues are being dealt with quickly).

mathieuboudreau commented 1 year ago

I started going through the checklist. Here I'll keep a running list issues or PRs in the repo that I've opened and if they've been completed/closed or not yet.

cncastillo commented 1 year ago

Hi sorry for not replying. I will start to go through the checklist as well.

Just wanted to mention that I had a problem during installation due to a package incompatibility caused by Flux.jl (in an environment with more things that required CUDA). I think this problem it is caused beacuse the Project.toml is a little blaoted. I know (by experience) that this is very annoying to fix, but I think it is necessary.

I also noticed that the package has no tests.

I will leave these issues while I see the rest:

mrikasper commented 1 year ago

Dear Felix @felixhorger and Carlos @cncastillo,

I noticed both in Felix' review comment here and one of Carlos' issues a more conceptual question that I wanted to discuss a bit: Both of you are emphasizing the importance of modularization and small packages with limited, but efficient functionality.

I understand the need for that and the elegance in replacing certain tools based on one's specific needs, but in the end, in order to solve a complex task, we have to combine these tools into an elaborate pipeline that needs to make specific choices and adapt to the idiosyncrasies of our use cases. Reconstructing simulated data from a nominal Archimedean spiral is something different than turning real data from a real scanner into images that should then be used in established neuroimaging tools for further analysis, like FSL DTIFIT for diffusion (which rely on NIfTI as input format).

This is what our submission is mainly about (a pipeline for...), in particular GIRFReco.jl, whereas MRIGradients.jl is more of a tool in the sense that you describe it. In other parts of neuroimaging, having complex pipelines standardized and published is definitely seen as valuable, for example fmriprep for functional MRI, and I don't see why image reconstruction would differ in this respect.

Is this maybe a Julia-specific preference? Or because of what the notion of a "package" is in Julia? Originally, our repository was not even a package, but rather a project, and I would say mainly for practical reasons, e.g., ease of installation, we changed this during the pre-review process. It does make sense to have it as a package, I think, because other deployment features also become standardized (e.g., documentation and testing, which you pointed out), but maybe this is where the different expectations come from?

Looking forward to your insights, Lars

cncastillo commented 1 year ago

Dear Lars @mrikasper,

To keep myself from overextending here, I will reply about modularization in BRAIN-TO/GIRFReco.jl#12.

I think there could be a naming issue, as the name of the package GIRFReco.jl gives the impression that it provides the tools to do a GIRF-corrected reconstruction from the user's data (which is conceptually different from the coils sens. and B0 map estimation, IO, etc.). For example, if I want to include the linear gradient self-terms and $k_0$ corrections to my MRF radial data (or spiral), it seems that the most useful package would be MRIGradients.jl, as I could not easily use GIRFReco.jl.

A name that more closely represents what the packages are meant to do (core functionality) is needed to set expectations correctly. For example, MRISpiralDiffusionReco.jl, and MRIGIRFCompensation.jl. Maybe the efforts in documentation, tests, CI, etc, need to go to the package currently named MRIGradients.jl, and the presented pipeline can be an example use case that integrates fancier coils estimation, B0 correction, IO, etc.

Cheers!

felixhorger commented 1 year ago

Regarding the reproducibility of original results from the paper: there are phantom and in vivo results. The in vivo results cannot be reproduced by the reviewers due to data protection (see @mrikasper's comment here). For the phantom results, there is no code available, maybe it makes sense to include that in the joss_example.jl, i.e. to produce exactly the same plot as in the paper on top of the existing ones?

felixhorger commented 1 year ago

I reckon this point

  • [ ] 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

is implicitly fulfilled because the code is on github, hence anyone can do 1), 2), 3)? Or is an explicit statement required @Kevin-Mattheus-Moerman?

Kevin-Mattheus-Moerman commented 1 year ago

@alexjaffray @felixhorger in terms of the "Community guidelines". We recommend that the authors create a dedicated CONTRIBUTING.md file in which the guidelines to contribute are spelled out. This file should be found in the main folder are should be linked to from the README.md. This website has some nice examples: https://contributing.md/example/.

@alexjaffray if you find the dedicated file "overkill" you can also instead have a simple section in your README where you outline how you wish folks to contribute to the project etc. So similar content but shorter. We leave this up to you.

alexjaffray commented 1 year ago

@Kevin-Mattheus-Moerman Thanks for the suggestion of adding community contributing guidelines, I'll take a look at some examples and will add a dedicated file, I think this will be helpful and make the README less crowded

Kevin-Mattheus-Moerman commented 1 year ago

@alexjaffray any updates on the above?

alexjaffray commented 1 year ago

Thanks for the reminder, we’re working on all the other issues that have been raised at the moment (and making good progress) but I’m happy to add a contributing guide. I’ll add it to our internal project board and update once it’s been pushed to our response branch.

On Thu, 2 Nov 2023 at 01:47, Kevin Mattheus Moerman < @.***> wrote:

@alexjaffray https://github.com/alexjaffray any updates on the above?

— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/5877#issuecomment-1790302657, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEFRPBOX3BWS2E2UZTCIPO3YCNMZ5AVCNFSM6AAAAAA5HHT7ZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJQGMYDENRVG4 . You are receiving this because you were mentioned.Message ID: @.***>

alexjaffray commented 1 year ago

@Kevin-Mattheus-Moerman I've added it in the latest commit as CONTRIBUTING.md. Let me know if this is alright. It's https://github.com/BRAIN-TO/GIRFReco.jl/pull/4/commits/8ce0b68e94723417585ced4db1a6455d53769e4e

Kevin-Mattheus-Moerman commented 12 months ago

@alexjaffray yes the contributing guidelines look fine now. Any other updates? Let us know when the reviewers can pick this up again.

Kevin-Mattheus-Moerman commented 11 months ago

@alexjaffray :wave: :point_up: Can the reviewers now resume their work, or are we still waiting for you to respond/implement changes? Thanks.

alexjaffray commented 11 months ago

@Kevin-Mattheus-Moerman We are working towards addressing all of the points the reviewers raised, the points are pretty sweeping but all valid. I think we should be good to pass it back to the reviewers with revisions complete in a week.

Kevin-Mattheus-Moerman commented 11 months ago

@alexjaffray okay, great, thanks for the update.

alexjaffray commented 11 months ago

@Kevin-Mattheus-Moerman update: We wanted to have it back to reviewers this week, but some internal deadlines came up for us. We are working on a complete revision, but this week is unlikely. We propose to shift our timeline later by one week.

Kevin-Mattheus-Moerman commented 11 months ago

@alexjaffray that is fine. Let us know when the reviewers can pick this up.

Kevin-Mattheus-Moerman commented 10 months ago

@alexjaffray can you please provide an update? Thanks

alexjaffray commented 10 months ago

@Kevin-Mattheus-Moerman we have been working to incorporate the latest comments from the reviewers here: https://github.com/BRAIN-TO/GIRFReco.jl/pull/4

Kevin-Mattheus-Moerman commented 9 months ago

@alexjaffray could you please provide an update on progress or perhaps indicate when you think you will be able to have the above finished?

alexjaffray commented 9 months ago

@Kevin-Mattheus-Moerman we are working through the test checklist here, and have implemented most of the other changes requested by the reviewers here https://github.com/BRAIN-TO/GIRFReco.jl/pull/4#issuecomment-1832685488. I imagine we will be ready to complete the above in 2 weeks.

alexjaffray commented 9 months ago

@Kevin-Mattheus-Moerman @cncastillo @mathieuboudreau @felixhorger we have finished with our revisions as of commit https://github.com/BRAIN-TO/GIRFReco.jl/commit/d3d59f7d2d189a38c5540278aa9e961e44781538. We have implemented most of the changes suggested and we feel it has resulted in a much improved package. We are very grateful for the feedback we received and have incorporated it. We have also implemented changes in MRIGradients to improve it alongside this submission.

We have decided to tackle the off-resonance estimation and the figure resolution issue as a next step outside of the scope of the review, but are looking forward to addressing them soon as well.

We look forward to hearing from all of you. Best, Alex, Tim, and Lars.

Kevin-Mattheus-Moerman commented 9 months ago

@cncastillo @mathieuboudreau @felixhorger could you resume your review at this point? Thanks!

cncastillo commented 9 months ago

Sure, congratulations to the authors for all the work!

For my part I will re-read the paper, and try to reproduce the results. If I remember correctly the in-vivo data is not publicly available, so I will try with the phantom data.

felixhorger commented 9 months ago

@editorialbot generate pdf

editorialbot commented 9 months ago

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

felixhorger commented 9 months ago

I think the package is good to go once https://github.com/BRAIN-TO/GIRFReco.jl/issues/25 is fixed. I'll take your word for the interpolation issue with the figure 😄 Congrats and great job @alexjaffray @nbwuzhe @mrikasper, really appreciate the amount of extra work you put in, this is a valuable contribution!

nbwuzhe commented 9 months ago

I think the package is good to go once BRAIN-TO/GIRFReco.jl#25 is fixed. I'll take your word for the interpolation issue with the figure 😄 Congrats and great job @alexjaffray @nbwuzhe @mrikasper, really appreciate the amount of extra work you put in, this is a valuable contribution!

Thanks again @felixhorger for your careful review. The fixes have been implemented in our latest PR here; kindly let us know your further comments.

mrikasper commented 8 months ago

Dear @cncastillo @mathieuboudreau @Kevin-Mattheus-Moerman,

We just wanted to check in whether you had any time to proceed with the review.

Please let us know if you encounter any more obstacles in running the code and reproducing the phantom results. We are happy to help the process in any way possible.

Thank you for all your work!

Kevin-Mattheus-Moerman commented 8 months ago

@cncastillo @mathieuboudreau :wave: it would be great if you could finalise your review at your earliest convenience to avoid further delay here. Thanks again for your help!!

mrikasper commented 8 months ago

@Kevin-Mattheus-Moerman We got some feedback from @cncastillo yesterday in the repo issues and are working on it at the moment.

Kevin-Mattheus-Moerman commented 7 months ago

@mathieuboudreau 👋 it would be great if you could finalise your review at your earliest convenience to avoid further delay here. Thanks again for your help!!

mathieuboudreau commented 7 months ago

@Kevin-Mattheus-Moerman Thanks for the ping! Will get it done tomorrow (Monday).

alexjaffray commented 7 months ago

@mathieuboudreau just be aware that we are currently in the process of responding to suggestions from @cncastillo regarding our example. If you could delay your review of that part by one day, that would be great so that we can complete this prior to your testing of the example. The rest of the code and paper is good to review.