-
-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: GIRFReco.jl: An open-source pipeline for spiral MRI reconstruction in Julia #5877
Comments
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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
Wordcount for |
|
Review checklist for @cncastilloConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
@editorialbot generate pdf |
Review checklist for @felixhorgerConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
The DOIs it found for the ISMRM abstracts actually work, amazing, I didn't know about this 👍 |
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. |
@alexjaffray could you please respond to the points raised by @felixhorger ? |
@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 |
Review checklist for @mathieuboudreauConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
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 |
@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. |
@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). |
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.
|
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 I also noticed that the package has no tests. I will leave these issues while I see the rest: |
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 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, |
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 A name that more closely represents what the packages are meant to do (core functionality) is needed to set expectations correctly. For example, Cheers! |
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). |
I reckon this point
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? |
Done! version is now v0.1.7 |
@editorialbot set 10.5281/zenodo.11075548 as archive |
Done! archive is now 10.5281/zenodo.11075548 |
@editorialbot check references |
|
@alexjaffray please can you work on the above potential DOI issues? |
@Kevin-Mattheus-Moerman the items with no DOI given are correct, there is no DOI available for those 5 items and we have cited them as the authors of the packages/platforms suggest. Regarding the two items which have a "possibly valid" DOI, the dois suggested are correct but do not link directly to the works. Using the links we provided provides a direct link to those conference abstracts. What would you suggest in this case? |
@alexander: For the two ISMRM Abstracts, I think it's more long-lasting to put the DOI, and the full document is only one click away then. Similar to how most publishers DOIs redirect to the HTML page with the paper abstract, not the full text or pdf.
Of course, if there is a second field for URL (besides DOI) you can use the full-text link there.
All the best,
Lars
…--
Lars Kasper, PhD (he/him)
***@***.***
MR Physicist
Toronto Neuroimaging Facility (ToNI), Department of Psychology, University of Toronto
325 Huron St, Toronto, ON M5S 3J7
phone: +1 416-978-7613 (office) / +1 416-946-0356 (scanner)
________________________________
From: Alexander Jaffray ***@***.***>
Sent: Tuesday, April 30, 2024 9:48:35 AM
To: openjournals/joss-reviews ***@***.***>
Cc: Lars Kasper ***@***.***>; Mention ***@***.***>
Subject: Re: [openjournals/joss-reviews] [REVIEW]: GIRFReco.jl: An open-source pipeline for spiral MRI reconstruction in Julia (Issue #5877)
@Kevin-Mattheus-Moerman<https://github.com/Kevin-Mattheus-Moerman> the items with no DOI given are correct, there is no DOI available for those 5 items and we have cited them as the authors of the packages/platforms suggest.
Regarding the two items which have a "possibly valid" DOI, the dois suggested are correct but do not link directly to the works. Using the links we provided provides a direct link to those conference abstracts. What would you suggest in this case?
—
Reply to this email directly, view it on GitHub<#5877 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADFUIX7ED2CSKWJCHH2RGXLY76OLHAVCNFSM6AAAAAA5HHT7ZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBVGM4DSNZRHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@Kevin-Mattheus-Moerman @mrikasper I've added the DOIs for the abstracts which have them (the two suggested above), and pushed to the repository. @Kevin-Mattheus-Moerman Let me know if that is all that's required or if I need to re-archive everything. |
@editorialbot check references |
|
@Kevin-Mattheus-Moerman Thanks for running that again, the references look good to me, the MISSING DOIs list is correct, these are indeed items without dois |
@editorialbot recommend-accept |
|
|
👋 @openjournals/bcm-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#5295, then you can now move forward with accepting the submission by compiling again with the command |
@alexjaffray it looks like we are good to proceed. However, I would add one minor comment. On the Zenodo archive listing the spelling for the author Kâmil Uludağ does not feature the special characters. It may be good to edit this person's name there to match how it is listed in the paper too. You can do as if you wish but we can proceed to process this for acceptance at this point. |
@editorialbot accept |
|
Ensure proper citation by uploading a plain text CITATION.cff file to the default branch of your repository. If using GitHub, a Cite this repository menu will appear in the About section, containing both APA and BibTeX formats. When exported to Zotero using a browser plugin, Zotero will automatically create an entry using the information contained in the .cff file. You can copy the contents for your CITATION.cff file here: CITATION.cff
If the repository is not hosted on GitHub, a .cff file can still be uploaded to set your preferred citation. Users will be able to manually copy and paste the citation. |
🐘🐘🐘 👉 Toot for this paper 👈 🐘🐘🐘 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
@Kevin-Mattheus-Moerman Everything looks good, and the DOI resolves! Thank you! |
@alexjaffray great. Well done and congratulations on this JOSS publication! And a special thanks to the fantastic reviewers!!! @cncastillo, @mathieuboudreau, @felixhorger |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! The Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
Submitting author: @alexjaffray (Alexander Jaffray)
Repository: https://github.com/BRAIN-TO/GIRFReco.jl
Branch with paper.md (empty if default branch):
Version: v0.1.7
Editor: @Kevin-Mattheus-Moerman
Reviewers: @cncastillo, @mathieuboudreau, @felixhorger
Archive: 10.5281/zenodo.11075548
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
@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:
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
The text was updated successfully, but these errors were encountered: