Comments (19)
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
paper.md
contains two authors but the list of authors inAUTHORS.rst
and on zenodo only contains one author. This should be corrected.paper.md
contains minor typos: laboraries, sptial alignment, carefully-deisgnedsetup.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 andmrivis
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.requirements.txt
containsscipy
, which is not mentioned under requirements in thesetup.py
file.- 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 thecluster_rows_in_roi
method. - 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. - The
color_mix
function doesn't seem to work correctly. Using the following code:I get the following error:path1 = '../../example_datasets/3569_bl_PPMI.nii' from mrivis import color_mix color_mix(path1, path1);
TypeError: '<' not supported between instances of 'tuple' and 'int'
- In the Alignment check - usage section, using the following code in a terminal doesn't show any figure/image:
There seems to be something missing. Is it assumed that people have already imported
from mrivis import checkerboard path1 = '/Users/Reddy/Desktop/image.nii' path2 = '/Users/Reddy/Desktop/another.nii' checkerboard(path1, path2) # square patches
matplotlib
in a particular way? Do we need to use the.show()
method afterward? mrivis
contains additionalutils
functions that are not included on the documentation homepage, such asscale_0to1
,threshold_image
androw_wise_rescale
. I recommend to add them to the API documentation as well.- 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
orview_set
. I recommend having a working demo notebook that shows the functionality with on the provided example data. - Some of the methods in the Visualization class have input parameters that are not listed in the docstring (see here or here).
Carpet
class contains the input parameternum_frames_to_skip
that isn't described in the docstring.- The
color_mix
function fromworkflow.py
has parameter description triplets (i.e.cmap
,overlay_image
andoverlay_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 thevoxelwise_diff
function. - The
color_mix
function is missing the description text in the docstring for the parametercolor_space
. voxelwise_diff
function accepts an input parameteroverlay_image
that is not described in the doc string.- 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 theaseg_on_mri
function, defined inworkflow.py
which is also neither tested, documented or explained. - The following functions are defined but never used in the code base:
check_bounding_rect
,get_freesurfer_subcortical_LUT
,_open_figure
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
-
The current version of
mrivis_demo_Collage_SlicePicker_classes.ipynb
contains the errorNo module named 'mrivis'
. I suggest correcting this by just rerunning the notebook. -
mrivis_demo_Collage_SlicePicker_classes.ipynb
is missing theimport numpy as np
command. -
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.
-
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 replaceplt.show(collage.fig)
withcollage.fig
. -
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.
-
The last command (
bounding_rect
) inmrivis_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
- Add an example figure for the collage, as well as for the
Carpet
function for 4D images toREADME.rst
to show the full functionality of the software. - 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. - 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
- I recommend to also create an example/demo notebook for the alignment check functionality of
mrivis
. - 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 asvisualqc
, 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
- 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);
mrivis_demo_vis_classes.ipynb
is still missing thenumpy as np
import. Without it, it will not finish as it is.- What's the difference between
mrivis_demo_Collage_SlicePicker_classes.ipynb
andmrivis_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
- The new carpet plot notebook example is very nice!
- 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.
- 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. - The
color_mix
function still contains the docstrings for the parameterscmap
,overlay_image
,overlay_alpha
which are no valid input parameters. You should delete those descriptions from the docstring. - About the unused methods, all fine with me.
- 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
-
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 -
Added the
numpy
import to notebook now -
there is NO difference between
mrivis_demo_Collage_SlicePicker_classes.ipynb
andmrivis_demo_vis_classes.ipynb
, just duplicates created from bad merge
Comments about the revisions
- 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.
- Okay
- 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. - removed the invalid params from docstrings
- great
- 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.
Related Issues (13)
- Cannot visualize checkerboard HOT 6
- Brach merge disappeared!
- Carpet plot notebook
- New functionality in Carpet
- Derived classes for func. and diffusion MRI from Carpet
- More visualizations in README and landing pages
- SlicePicker from 4D volumes
- how not to assume orientation is encoded correctly HOT 1
- No checkerboard visible in output image HOT 15
- additional sampler options (string descriptions and within ROI)
- New Carpet class for fMRI and dMRI HOT 2
- Slicepicker export to video or GIF HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from mrivis.