Review for Journal of Open Source software about mrivis HOT 19 CLOSED

Thanks for the detailed review Michael, closing this for now, feel free to reopen if I missed opening separate issues for things we identified.

from mrivis.

Thanks Michael for taking the time to review this in great detail.

Does this mean you accept the software as is publication? Or are you planning to open any new issues for your suggestions?

from mrivis.

cc @arokem

from mrivis.

Sorry @raamana, you were a bit quicker than I expected :-) I have a list of minor revisions and comments that I will post in the next hour but I wanted to separate it from my general feedback.

from mrivis.

The following are my comments and issues that I encountered while reviewing mrivis. I apologize if it is too thorough and I want to iterate again that I think that the software is very well written, has a nice documentation and is clearly needed.

Minor Revision

1. paper.md contains two authors but the list of authors in AUTHORS.rst and on zenodo only contains one author. This should be corrected.
2. paper.md contains minor typos: laboraries, sptial alignment, carefully-deisgned
3. setup.cfg contains "current_version = 0.1.0", this should be updated to the current version 0.3.0. HISTORY.rst only contains version 0.1.0. For completeness, I recommend updating this to version 0.3.0 and mrivis is listed on PyPI (here) as version 0.3.5 but everywhere else as 0.3.0. Make sure that the versions are everywhere the same.
4. requirements.txt contains scipy, which is not mentioned under requirements in the setup.py file.
5. I highly recommend to also include a demo of the Carpet class in a notebook. This functionality is very useful! I additionally recommend to also include an example to the cluster_rows_in_roi method.
6. Should the carpet plot approach be inspired or related to the paper from Power et al. (2016) then I recommend to cite it in paper.md as well.
7. The color_mix function doesn't seem to work correctly. Using the following code:

   path1 = '../../example_datasets/3569_bl_PPMI.nii'
   from mrivis import color_mix
   color_mix(path1, path1);

   I get the following error: TypeError: '<' not supported between instances of 'tuple' and 'int'
8. In the Alignment check - usage section, using the following code in a terminal doesn't show any figure/image:

   from mrivis import checkerboard
   
   path1 = '/Users/Reddy/Desktop/image.nii'
   path2 = '/Users/Reddy/Desktop/another.nii'
   
   checkerboard(path1, path2) # square patches

   There seems to be something missing. Is it assumed that people have already imported matplotlib in a particular way? Do we need to use the .show() method afterward?
9. mrivis contains additional utils functions that are not included on the documentation homepage, such as scale_0to1, threshold_image and row_wise_rescale. I recommend to add them to the API documentation as well.
10. Running the example in the Alignment check - usage section doesn't necessarily lead to nice looking figures as one might need to adapt additional parameters, such as background_threshold or view_set. I recommend having a working demo notebook that shows the functionality with on the provided example data.
11. Some of the methods in the Visualization class have input parameters that are not listed in the docstring (see here or here).
12. Carpet class contains the input parameter num_frames_to_skip that isn't described in the docstring.
13. The color_mix function from workflow.py has parameter description triplets (i.e. cmap, overlay_image and overlay_alpha are explained three times). This even though none of these parameters are actual input parameters to this function. It seems that the description text of those three parameters belong to the voxelwise_diff function.
14. The color_mix function is missing the description text in the docstring for the parameter color_space.
15. voxelwise_diff function accepts an input parameter overlay_image that is not described in the doc string.
16. Functions in color_maps.py are never used in any example/demo, isn't tested or mentioned in the documentation. It seems that they are only needed for the aseg_on_mri function, defined in workflow.py which is also neither tested, documented or explained.
17. The following functions are defined but never used in the code base: check_bounding_rect, get_freesurfer_subcortical_LUT, _open_figure
18. README.rst on github contains the section If you would like to use the export to GIF from SlicePicker, then install the imageio package as well via pip install imageio. This section is not included in the docs under installation. I recommend to add this section to the documentation. Perhaps also include an example on how to create a GIF in the demo notebook.

Tutorial/Documentation revision

1. The current version of mrivis_demo_Collage_SlicePicker_classes.ipynb contains the error No module named 'mrivis'. I suggest correcting this by just rerunning the notebook.

2. mrivis_demo_Collage_SlicePicker_classes.ipynb is missing the import numpy as np command.

3. For the plotting of two subplots in mrivis_demo_Collage_SlicePicker_classes.ipynb to work, the two cells...

   # lets have two subplots
   fig, ax = plt.subplots(2, 1, squeeze=True)

   and

   # you just need to pass in all the
   for s1, s2 in sp.get_slices_multi([img_one, img_two]):
       ax[0].imshow(s1)
       ax[1].imshow(s2)
       sleep(0.1)
       plt.pause(0.001)
   #
   # PS: You might not be able to see the animation in the jupyter notebook.

   need to be combined. Otherwise, this doesn't work in a notebook.

4. The collage command in mrivis_demo_Collage_SlicePicker_classes.ipynb does not work in a notebook if the three first cells are separated. The first cell creates an empty canvas that cannot be accessed by the following cells. Either combine the three cells into one or replace plt.show(collage.fig) with collage.fig.

5. The notebook contains the phrase: "Coming: vary the number of slices per each view (this is implemented, needs to be tested more)". I recommend to remove this sentence and to open a corresponding issue on github.

6. The last command (bounding_rect) in mrivis_demo_Collage_SlicePicker_classes.ipynb doesn't seem to work correctly in the notebook. The collage is not constraint to the top right corner. The command seems to work if the Python code is run from a terminal, but the collage doesn't seem to be sized correctly neither, as the right part of the images is cut off/reaches over the canvas.

Optional changes

1. Add an example figure for the collage, as well as for the Carpet function for 4D images to README.rst to show the full functionality of the software.
2. The demo notebook(s) could also be made available via binder.org. Such a service would allow showcasing mrivis's functionality directly on the web. No additional setup would be required.
3. The documentation homepage has a usage and API section for the alignment check but only an API section for the Visualization classes. I would recommend having also a usage section for the Visualization classes
4. I recommend to also create an example/demo notebook for the alignment check functionality of mrivis.
5. The codebase contains 14 TODO comments. I recommend to either solve this todo's or create individual issues for them. This helps to quickly finish those points or to make others aware of them.

from mrivis.

Thanks Michael, this is pretty detailed and helpful. I'm revising it now and should be done soon.

Few questions:

• option of bounding_rect in Collage is buggy - I spent sometime before trying to make it more reliable, but found matplotlib docs to be not helpful enough and I anticipate this may MUCH MORE effort to debug. Would it okay if I list it as a known issue for now (in docs/API etc) and flag it for future debugging.
• color_maps module is used by other libraries such as visualqc, and I included them here as they are more visualization oriented than QC oriented. Perhaps mentioning them in the list of features would be helpful?

from mrivis.

Happy to hear that it helps!

• I think the bounding_rect (almost?) worked if it's run directly from Python code (i.e. not with Jupyter notebook). Therefore, if you flag it as a future debug issue and just mention in the demo notebook that it doesn't work in the notebook, that's alright with me. It's a really cool feature but not essential for the toolbox to be shipped.
• Sorry, I didn't consider that color_maps might be needed by other libraries. I have no problem if you keep it in there, I just wanted to point out that this might be either some legacy code or hasn't grown to its full potential yet. So if you mention this as an additional feature, even better 👍

from mrivis.

I think I have mostly addressed your comments now, including many of the optional ones. Take a look.

PS nbviewer does not seem to be so reliable, my binder seems to work, but if they don't show up, its likely an issue on their side.

Future work:

• Yes, a notebook for alignment check would be great
• Don't worry about the unused methods for now (like get_freesurfer_subcortical_LUT, _open_figure, aseg_on_mri, ). I am either using in other libraries or will soon remove them
• TODO is my preferred way to track todos :). I will try opening them as issues when I get more time.

from mrivis.

hey @raamana - great! I think we can finish this rather quickly.

Needs to be addressed

1. The color_mix function (as described here still doesn't seem to work correctly. Should the following code work or do I need to run it differently?

    path1 = 'example_datasets/3569_bl_PPMI.nii'
    path2 = 'example_datasets/4086_bl_PPMI.nii'
    from mrivis import color_mix
    color_mix(path1, path2);

2. mrivis_demo_vis_classes.ipynb is still missing the numpy as np import. Without it, it will not finish as it is.
3. What's the difference between mrivis_demo_Collage_SlicePicker_classes.ipynb and mrivis_demo_vis_classes.ipynb? The notebooks seems to be exactly the same. If they are the same, you should delete one and make sure to only link to this one throughout the documentation.

Comments about the revisions

1. The new carpet plot notebook example is very nice!
2. Having an alignment notebook would be very nice. But you describe very well how one can use it in the documentation, so it's not a requirement for the paper.
3. Stephen C. Strother is still not mentioned under AUTHOR.rst or on zenodo. But as you mention him in the JOSS paper, that's ok for me.
4. The color_mix function still contains the docstrings for the parameters cmap, overlay_image, overlay_alpha which are no valid input parameters. You should delete those descriptions from the docstring.
5. About the unused methods, all fine with me.
6. It's ok for me if you leave the TODO in the code, I understand this approach very well :-) Just thought that if you create issues, others might see the problematic and might be able to work on it themselves. Also, I wanted that you verify that those are not relevant for the JOSS paper.

Concerning mybinder (offtopic)

Off-topic:
What do you mean about nbviwer is not reliable? I never had problems with it. But yes, with mybinder, all the time.

As an additional note, your current notebook will not work in mybinder because you include your personal absolute path base_dir = '/Users/Reddy/dev/mrivis/example_datasets'. I recommend to change this in the notebook to this base_dir = '../../example_datasets/'. Like that, the mybinder notebook will also work without applying any changes.

from mrivis.

Thanks Michael for the prompt and thorough checks.. Sorry, I thought I made the changes necessary for the above, apparently not.

Needs to be addressed

1. The color_mix function (as described here is not supposed to work as shown. I now left more instructions on how to make it work

2. Added the numpy import to notebook now

3. there is NO difference between mrivis_demo_Collage_SlicePicker_classes.ipynb and mrivis_demo_vis_classes.ipynb, just duplicates created from bad merge

Comments about the revisions

1. Thanks - carpet plot notebook needs to be significantly expanded in the future with real world use-cases, and with additional functionality such as blurring within ROI etc. Some other options I plan to add are composite layering of stats (as in visualqc), and mixing multiple variations of carpet (temporal derivatives etc).This would require significant efforts, and I am looking for more time (on my part) and potential contributors. Just opened issues for these ideas.
2. Okay
3. Stephen C. Strother is still not mentioned under AUTHORS.rst as its more a list of who contributed code (and as it is mentioning only Dev Lead). SCS has contributed to design, conception of useful functionality and financially supporting the work. He has not contributed code yet, so didn't want to add his name. I could perhaps delete this file if its confusing.
4. removed the invalid params from docstrings
5. great
6. Just checked TODOs - they do not interfere with the advertised functionality. They are more of a future TODOs for me.

Off-topic:

• nbviewer was throwing the 404 not found error - likely due to delays in syncing across GitHub and nbviewer
• fixed the hardcoded paths in notebook, it should now work on mybinder (after they get synced and relaunched)

from mrivis.

Thank you @raamana - this seems all good to me!

Irrelevant for the paper, but just that you know. The deleted mrivis_demo_Collage_SlicePicker_classes.ipynb string is still appearing under

• docs/example_notebooks/mrivis_demo_vis_classes.ipynb
• docs/readme.rst
• docs/classes.rst

from mrivis.

Just double checked, the binder works :)..

from mrivis.

do you see the deleted notebook here: https://github.com/raamana/mrivis/tree/master/docs/example_notebooks ?

from mrivis.

that might be because my changes pushed haven't reached GitHub when you checked ;)

from mrivis.

No, the deleted notebook is gone. But when I do a grep -r "mrivis_demo_Collage_SlicePicker_classes" in the downloaded source code, I'll still find the old notebook at those three places.

from mrivis.

did you pull the latest before greping?

from mrivis.

They are hidden files and checkpoints (not a concern for users).. I've updated .rst's on gh-pages branch (which is what goes on the docs website), but I will try clean them up on master too.

$ 10:28:15 SQuark-3 mrivis >>  grep -rl "mrivis_demo_Collage_SlicePicker_classes" .
./docs/example_notebooks/mrivis_demo_vis_classes.ipynb
./docs/example_notebooks/.ipynb_checkpoints/mrivis_demo_Collage_SlicePicker_classes-checkpoint.ipynb
./docs/example_notebooks/.ipynb_checkpoints/mrivis_demo_vis_classes-checkpoint.ipynb
./docs/classes.rst
./docs/_build/html/_sources/readme.rst.txt
./docs/_build/html/readme.html
./docs/_build/doctrees/readme.doctree
./docs/readme.rst
$ 10:28:25 SQuark-3 mrivis >>

from mrivis.

Sorry, I didn't mean the files in .ipynb_checkpoints or _build but for example docs/readme.rst on master that creates a nbviewer 404 error here if you click on the demo button.
I was just worried that they have a direct influence on gh-pages.

from mrivis.

Thanks for spotting it - fixed the link in docs/readme.rst in master now, although I hope users will go through the website only :)

from mrivis.