Closed whedon closed 3 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @sneakers-the-rat, @pauldmccarthy 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:
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
PDF failed to compile for issue #2670 with the following error:
Can't find any papers to compile :-(
Hi all! π Thank you so much, @sneakers-the-rat, @pauldmccarthy for accepting to review this. Please read the instructions above. Any questions, feedback on the paper, etc., please post here. Any very code-specific questions, suggestions, etc., please use the issues in the code repo and link to them from this thread so we can all keep track of them. πΈ
For an example of how this process plays out feel free to skim previous reviews, such as: #2285 and #2348. βΊοΈ
@whedon generate pdf from branch joss-paper-branch
Attempting PDF compilation from custom branch joss-paper-branch. Reticulating splines etc...
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@whedon generate pdf from branch joss-paper-branch
Attempting PDF compilation from custom branch joss-paper-branch. Reticulating splines etc...
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hey @sneakers-the-rat, @pauldmccarthy β could you please let us know when your reviews might be ready by?
Hi @oliviaguest, I'm aiming to complete my review in the next 7 days.
@whedon remind @pauldmccarthy in 2 weeks
Reminder set for @pauldmccarthy in 2 weeks
yes give me the same - setting reminder for 10/7
@whedon remind @sneakers-the-rat in 2 weeks
Reminder set for @sneakers-the-rat in 2 weeks
@sneakers-the-rat you got it! β¨
may need an extra day or two but i am with ya
nvm just finished <3
loved it! very straightforward to review, tool worked as expected and was quite intuitive. I made a few suggestions (wasn't sure where to put them) and raised a few issues but nothing that would prevent me from accepting the paper according to the checklist & claims in the paper.
Olivia let me know if i've messed anything up/need to do anything further. Katja & Roberto, thanks for this tool!
@sneakers-the-rat Amazing! Can you do me a favour and move what you wrote into a comment in the issue as opposed to into the OP? I'd prefer it if everything to-be-addressed was a comment here to make it easier to keep track of. βΊοΈ
PS: @sneakers-the-rat if you need examples/inspiration, check: #2285 and #2348. πΌ
gotcha, will clean up op in 1 sec. only copying things here where i've made a comment
tbc none of these are comments that i think are worthy of holding up the manuscript, very little touches that don't get in the way of the central function of the program
Functional claims from manuscript:
Potential bugs question mark?
ux feedback/"would be nice" recommendations:
strongly recommend adding progress bars when loading/saving images - the page becomes unresponsive while loading/saving and it wasn't clear to me if the page had crashed or if i just needed to keep waiting.
an additional set of handles for translation/rotation in addition to dragging. the current handles seem to move the image in the direction normal to the plane, but it would be nice to be able to do the 'dragging' controls with handles for when eg. the rotation doesn't behave exactly as expected.
'reset selection box' button
'load new nifti' button rather than having to refresh
would be nice to export matrix in different formats like csv, npy, etc. (unless this .mat format is universal across libraries, which i don't know)
display metadata of file, mostly in order to give some sense of scale to the matrix/crop box but also to keep track of what file you're working on if you're working on a bunch of similar-looking images
keep filename when saving, perhaps appending _reoriented
rather than always defaulting to just reoriented.nii.gz
for the same reason as ^^
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
reorient.mat
and selection.txt
standard file formats? what are each of the numbers in them? I get that they're an affine transformation matrix & crop box but would be good to document the format. additional details on the output image file (or even just saying that the image is unmodified except for transformation/cropping) would also be useful.[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
Test Reorient
Unit tests
eye()
β should return identity matrix
alpha(Ο/2)
β should return a 90 deg X rotation matrix
beta(Ο/2)
β should return a 90 deg Y rotation matrix
gamma(Ο/2)
β should return a 90 deg Z rotation matrix
matrix2str
β should return a string representation of a matrix
End to end tests
Load a file
β should display title
β should display "Choose..." message
β init with test nifti file (672ms)
β has the expected dimensions (107ms)
β has the expected datatype
β can rotate the image (135ms)
β can translate the image (96ms)
12 passing (2s)
config.yml
and chromium fails to launch. not sure if there are code coverage requirements for JOSS or just the existance of CI, but can't verify coverage. tests are a little barebones (eg. tests with single hardcoded values from example image file rather than programmatically generated tests/stress tests) but they are there and seem to test what they purport to..circleci/config.yml
file rather than in the package.json
dependencies? The http-server
should be started by the npm test
command rather than separately if it's required for the tests to work, would recommend something like
"scripts": {
"test": "http-server . & sleep 2 && mocha --exit"
},
[x] 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
@sneakers-the-rat perfect! @katjaq let us know when you have checked these out and addressed them. βΊοΈ
@pauldmccarthy was this just one you missed out on accidentally?
State of the field: Do the authors describe how this software compares to other commonly-used packages?
@oliviaguest, No, I wanted to re-read the article/docs before signing off on everything - planning to finish it off tonight!
@whedon generate pdf from branch joss-paper-branch
Attempting PDF compilation from custom branch joss-paper-branch. Reticulating splines etc...
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Apart from a few minor issues and suggestions (listed below), I think this is a really neat piece of work!
There is just one point made in the manuscript which I would like clarification on:
The affine matrix and selection box can be used later within a scripted workflow
I couldn't find any further mention of this point in the manuscript, nor in the source repository or app help. I presume the intent is that other tools (e.g. fslroi
, flirt
, etc - highlighting my bias towards FSL there) would be used for using the outputs in a scripted workflow?
I'm not necessarily requesting any changes to the manuscript or tool documentation, but it was just something which stood out to me, and which I wasn't sure what to make of, so was hoping for this point to be clarified.
Thanks!
[addendum, after looking at the extremely comprehensive review by @sneakers-the-rat] indeed, it may be worth adding an overview somewhere in the help documentation (I don't think this belongs in the manuscript) about how to use the selection box/affine files "off-line", i.e. outside of the reorient web interface, in an automated pipeline.
Unfortunately there are substantial differences across the different neuroimaging packages in the ways that files/information such as these are applied. I suppose the assumption is that if a user wished to use these outputs in their own workflow, they would know how to apply them using their tools of choice, which is fair enough. But I feel that, given that it is mentioned in the manuscript, it probably should be mentioned, if only briefly, somewhere in the documentation :)
@sneakers-the-rat, @pauldmccarthy thank you both so much! Your reviews look very detailed and useful. Please, @katjaq tag me when you have had a chance to discuss and address these. βΊοΈ
hello! quick update. We've pushed several commits to address the issues left by @sneakers-the-rat and @pauldmccarthy. There's in particular a python script (reorient.py
) which can take the original MRI file, plus the affine matrix and the selection box produced by the Web app, and reproduce the result of "Save Nifti" from the interface (so that there's no need for the Web app for just re-running the process automatically).
We still want to update the doc to take into account some of your comments, and then will draft a detailed answer :)
@r03ert0 excellent β sounds like you have your work all planned out. β¨
π @katjaq @r03ert0 π
Just a little bump for this review thread: is everything going OK?
Hello :)
A huge thank you to the reviewers for their amazing detailed reviews. We addressed all of the points mentioned, implemented the suggested add ons, and included more info in the doc as well as a pointer to the doc in the Readme. Please find the detailed answers to each issue below.
Q: "append to existing rotation matrices i'm not entirely sure what it means to append an affine matrix, so i'm not sure what i should be expecting when i click it, but something happens that hopefully makes sense to the authors." A: Each MRI volume has its own affine matrix. The options are to change original_matrix into loaded_matrix, or original_matrix into original_matrix*loaded_matrix. This information was added to the doc.
Q: "Fails without notification for image in MNI space -- since there is no description in manuscript of supported/unsupported formats/filetypes/etc. I don't consider it a bug for the purposes of the review, but that would be a good thing to document & failures like this should give some indication to the user even if the format isn't supported. raised issue at neuroanatomy/reorient#6" A: Accepted file formats are now checked, and an alert displayed in case of incorrect format. Additionally, why added the possibility of reading uncompressed nii files in addition to nii.gz (Commit e2e6eb9)
Q: "strongly recommend adding progress bars when loading/saving images - the page becomes unresponsive while loading/saving and it wasn't clear to me if the page had crashed or if i just needed to keep waiting." A: We added a message to show uploading, and better error message in case of failure (commit 7ad208f)
Q: "an additional set of handles for translation/rotation in addition to dragging. the current handles seem to move the image in the direction normal to the plane, but it would be nice to be able to do the 'dragging' controls with handles for when eg. the rotation doesn't behave exactly as expected." A: Translation handles would be a good idea, but we haven't found a way of making a good controller for that. The traditional way of doing translation is by clicking on the center of the selection box and dragging. However, in our experience it happened that we could translate instead of move a single side by mistake, which then requires to re-adjust the whole selection. Our current approach may take a bit longer, but it's always safer... Concerning Rotation handles, our focus is currently to align the MRI to the orthogonal anterior-posterior/left-right/superior-inferior axes. A general tool for reorienting with arbitrary directions could be useful in some other contexts, however.
Q: "'reset selection box' button" A: added in commit d9e5fd7
Q: 'load new nifti' button rather than having to refresh" A: We discussed this at length. It would be very simple to add, but we actually prefer the reload, and having one button less in the interface. However, we do explicitly state in the documentation now that a "reload" is requited for working with a new MRI. (commit c6f8ec9)
Q: "would be nice to export matrix in different formats like csv, npy, etc. (unless this .mat format is universal across libraries, which i don't know)" A: The .mat format is widely used in neuroimaging. It is actually just a text file with four rows of 4 numbers separated by spaces, and can be easily read using python or other languages. We included a python script for reproducing a reorientation offline (reorient.py, commit 6083a41). This info is added now to the doc as well.
Q: "display metadata of file, mostly in order to give some sense of scale to the matrix/crop box but also to keep track of what file you're working on if you're working on a bunch of similar-looking images" A: We now display the file name on top of the MRI viewers.
Q: "keep filename when saving, perhaps appending _reoriented rather than always defaulting to just reoriented.nii.gz for the same reason as ^^" A: This could be a feature, or a bug... In our case, we keep subject ID only in the directory name, and all files inside that directory have standard names (t2.nii.gz, dwi.nii.gz, etc). We like it because scripts look cleaner. Adding the filename to the reorient.mat or selection.txt files would force us to manually remove the file name every time... It's true that some people may have the opposite approach, and will have to manually add the names... we took a selfish decision...
Q: "reorientation matrix and selection box can be obtained in 1m for a typical brain" A: Typical MRI file sizes go from ~10 MB to about 100 MB. In animal MRI sometimes file sizes are larger, and we do have some files which are 300 MB. We haven't tried with larger files, and it is a nice surprise to know that the browser didn't break for 1GB . In case of a loading failure, we now check if the file is too large, and provide that information as feedback to the user (commit 7ad208f).
Q: " i think the docs should be put in the README" A: We added a link to the documentation to the Readme file. Users do not need to go to the github repo to use the tool, so they may never see the README file there. The "?" tab in the tool links to the documentation from the tool :)
Q: "what the displayed matrices are, eg. i'm not sure what 'world to voxel' vs. 'voxel to world' means" A: This is the standard way of naming in neuroimaging a matrix that would convert voxel indices (integer numbers starting from 0) into millimeters : that's the voxel_to_world. The world_to_voxel matrix does the inverse: it takes a coordinate in millimeters from real (world) space into voxel indices. Because world coordinates can be anything, they may not map inside the voxel volume, in which case the voxel index coordinates would be either negative, or larger than the volume dimensions (in voxels). This information is now also included in the doc.
Q: "what the format of the generated files are: are the reorient.mat and selection.txt standard file formats?" A: The .mat format is standard (although it is just a text file). The selection.txt format is not. The first row in the selection format represents the coordinates of the "minimum" corner, the second row the coordinates of the "maximum" corner. We now provide a python script that can take the source MRI, the reorient.mat file, and the selection.txt file to produce the reoriented volume (commit 6083a41). This info has also been added to the doc.
Q: "what the anatomical axes are:" A: LR is left/right, AP is anterior/posterior, SI is superior/inferior.
Q: "is there a reason why mocha is specified separately" A: Not really... but we added now the "test" call to the npm package as suggested.
Q: "lack of API-level documentation" A: We used jsdoc-style comments which could allow us to generate an API documentation.
Q: "The affine matrix and selection box can be used later within a scripted workflow" A: reorient.py script added (commit 6083a41). We also added an offline workflow description to the doc.
Q: it may be worth adding an overview somewhere in the help documentation (I don't think this belongs in the manuscript) about how to use the selection box/affine files "off-line", i.e. outside of the reorient web interface, in an automated pipeline. A: We added a "Reproduce the workflow offline" entry in the doc.
Finally, we addressed some other issues that had been directly added in reorient's repo. In particular, @pauldmccarthy had found a bug in our data display that was evident when the volumes were very small (-> voxels very big). We fixed the issue, and tested the interface extensively: https://github.com/neuroanatomy/reorient/issues/4
Thank you very much again for reviewing our tool! Katja & Roberto
@whedon generate pdf from branch joss-paper-branch
Attempting PDF compilation from custom branch joss-paper-branch. Reticulating splines etc...
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@katjaq @r03ert0 can you deposit the code on zenodo or similar and post the DOI here, please?
@katjaq @r03ert0 Has neuroanatomy/reorient/issues/9 been addressed? Can you close if so.
Hello, issue #9 has bee addressed and is now closed. Thanks.
The code is deposited on Zenodo under DOI 010.5281/zenodo.4165153
Thank you!
@whedon set 010.5281/zenodo.4165153 as archive
010.5281/zenodo.4165153 doesn't look like an archive DOI.
@whedon set 10.5281/zenodo.4165153 as archive
OK. 10.5281/zenodo.4165153 is the archive.
@katjaq @r03ert0 great! can you also change the title of the zenodo repo to the same as the JOSS paper, please? βΊοΈ
Oh, and is the version number at the top correct? If not, what's the current version?
Done! I updated the title to match that of the paper. The DOI which appears in the Zenodo badge in reorient's repo is automatically set to the one of the latest version (10.5281/zenodo.4165154). The DOI Katja sent is the umbrella DOI for all versions (10.5281/zenodo.4165153). It's better to use that one, no?
@katjaq @r03ert0 I mean this version here in the OP, get me?
@whedon accept
Attempting dry run of processing paper acceptance...
Submitting author: @katjaq (Katja Heuer) Repository: https://github.com/neuroanatomy/reorient Version: v1.0.0 Editor: @oliviaguest Reviewer: @sneakers-the-rat, @pauldmccarthy Archive: 10.5281/zenodo.4165153
: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 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
@sneakers-the-rat & @pauldmccarthy, 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 @oliviaguest 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 @sneakers-the-rat
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @pauldmccarthy
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper