switch from papaya to brainsprite in plotting.view_stat_map

[pbellec]

I really love the new 3D interactive viewer (plotting.view_stat_map), but the notebooks it is producing are huge. In this PR, I am proposing to switch from papaya to brainsprite, which is a js library I developed for the exact purpose of embedding lightweight 3D viewers in html pages (http://github.com/simexp/brainsprite.js).

The first difference with papaya is that it is using a jpg or a png containing all sagital slices of a volume as well as json metadata to store the brain images. That tend to be quite smaller than a nifti (depending on the numerical precision of the nifti). That also means that brainsprite can render brains with core html5 features, and no dependencies. So the brainsprite library weighs 15kb (500 lines...), as opposed to 2Mb for the current papaya html template. I have attached two brain viewers embedded in jupyter notebooks. The Papaya-based notebook is 12Mb, while the brainsprite-based notebook is 500kb. Again, this reflects a core difference in design: papaya is a full brain viewer app, featuring nifti reading as well as colorbar etc. Brainsprite is a minimal, fast brain viewer working from a pre-generated sprite.

Which makes a transition with the second point: all the action for the generation of the brain volume happens in python. There is a new function called save_sprite that generates the brain sprite as well as the json meta data. It relies on matplotlib, as well as nilearn's own functions. In particular, thresholding and colormap generation are all done with nilearn's code. Resampling as well. This means that it will be easier to maintain and evolve for nilearn's developpers. The current version replicates all the arguments of plot_stat_map, including draw_cross, annotate, cut_coords and a few other (with a few as bonus, such as opacity).

This PR is far from polished, there are a few oustanding issues, here. I also need to look into the doc and testing. Finally, I dumped some functions in html_stat_map.py which should probably live elsewhere. But I think it is time to get feedback, and in particular I'd like to know if there is an interest in merging this PR at all...

[cdansereau]

That would be a great feature to add.

[GaelVaroquaux]

I just tested the functionality, and it works very well. In addition to the memory savings (which are a big deal), I also prefer the look-and-feel. The fact that it is very similar to what the other plotting functions give is very useful. In particular the display of the cut coordinates. Thank you! A few comments on the look and feel: - Could we have the same default behavior as plot_stats_map: threshold being set to 1e-6, to cut out near zeros. - The IFrame is now too tall. Can you make it smaller, so that it fits closely the plot. This more compact view will be, by itself, a good gain. Indeed, I found that working in a complex jupyter notebook with those very large plots was not very convenient. Maybe more later. I need to switch to something else.

[pbellec]

@GaelVaroquaux great to hear you like it!

I ran the code through http://pep8online.com/ and fixed all detected issues.
It's updated now.

For the iframe, I will make it fit (and try to adjust size to screen, currently even the papaya viewer is broken when visualized on a phone).

For the threshold, I tried but somehow the result is inconsistent with plot_stat_map. I'll dig further.

As I said there are lots of work left, from the tests to the doc, to other minor things listed here.
Will resolve everything asap.

[jeromedockes]

this will be a great improvement. thanks a lot! I'll add some more detailed comments later this week

[pbellec]

I think I solved the fit of the iframe. Check this example to see how it looks now.

To solve this issue, I had to change the code of js_plotting_utils.py a bit. Before the size of the iframe was set to 600 x 400 (in pixels), by the width and height properties. That did not work with brainsprite (aspect ratio is more 10:5 than 3:2), and also did not scale to, say, a phone.

With the new code, width is expressed as a percentage of the parent element on the page (typical range 0-100, although one may want to work with width >100).

height has disappeared, and is replaced by ratio, which is height/width, in percentage (again, possible to go beyond 100% to get portrait orientation).

I've set the default width at 75%, and the default ratio at 68%. I've checked and view_connectome and view_surf seem to behave properly. The result is quite similar to the original 600 px x 400 px.

Note that the width and ratio parameters are only supported in the iframe and the notebook.
If they play a role elsewhere (or height), I probably broke that :)

Regarding the code, the action happens here
It is a pure css solution. If you are curious, this post explains how it works. Because jupyter is using bootstrap, I was able to simply use a bootstrap class to get the desired behaviour, and the code is quite concise. I could also implement a pure css style, the code would just be longer.

Finally, when it comes to brainsprite, after this change to js_plotting_utils, all I needed to do was specify the right aspect ratio when I create the viewer. It is done on this line.

[jeromedockes]

I was able to simply use a bootstrap class to get the desired behaviour, and the code is quite concise. I could also implement a pure css style, the code would just be longer

I think I would prefer to write the style rather than use a bootstrap class, in case someone wants to put the brainsprite in a webpage that doesn't use bootstrap

[jeromedockes]

could we allow 4d images with only one frame (note: the current behaviour is that of master, but other functions allow it)

[pbellec]

is there a simple helper function for that, like check_niimg_3d ?

[jeromedockes]

please use check_threshold from utils.param_validation

[pbellec]

So I checked this function. The syntax is a bit awkward, as it forces me to also import a function percentile_func. And as far as I understand, the behavior of threshold would change, as it would allow for threshold like "90%", which is not documented. I am tempted to put that off as future amelioration. What is currently implemented actually fits the doc (and plot_stat_map).

[pbellec]

Now I see it's included in the tests. I'll fix that.

[pbellec]

OK so I have added the feature, as well as a short description in the doc of threshold.

[jeromedockes]

please keep the symmetric_cmap option to enable plotting anatomical images

[GaelVaroquaux]

This has been addressed, hasn't it?

[jeromedockes]

This has been addressed, hasn't it?

no, symmetric_cmap is still missing, which is required to plot anatomical images

[jeromedockes]

why did the jquery version need to change?

[jeromedockes]

please run flake8 again, there are a few things to fix, in particular unused imports and variables

[jeromedockes]

this breaks if a string is given, because find_cut_coords is passed a string. please use check_threshold

[pbellec]

was also brought up by @kchawla-pi . Will work on that.

[pbellec]

mmhh, now I realize I did not understand the doc. I thought str referred to the case 'auto'. But what you are saying is that threshold="90%" should be supported?

[jeromedockes]

I sometimes see some weird gray pixels, what are they due to?

to reproduce:

>>> data = datasets.fetch_localizer_button_task(get_anats=True)
>>> img = data['tmaps'][0]
>>> anat = data['anats'][0]
>>> v = plotting.view_stat_map(img, threshold=2., bg_img=anat)

[jeromedockes]

actually, i also see some such pixels in papaya viewer with master, but they are harder to see. they don't appear in matplotlib plots

[pbellec]

I sometimes see some weird gray pixels, what are they due to?

to reproduce:

>>> data = datasets.fetch_localizer_button_task(get_anats=True)
>>> img = data['tmaps'][0]
>>> anat = data['anats'][0]
>>> v = plotting.view_stat_map(img, threshold=2., bg_img=anat)

Actually I believe this is the same issue I noticed when I tried to implement the default threshold at 1e-6. The result is super weird (see image attached comparing view_stat_map and plot_stat_map.

This may have to do with the fact that the functional volume is resampled from native space to the background space (using isotropic voxels) before the threshold is applied. This may introduced some interpolation artefacts. But you are saying that it does not show up in matplotlib, so that suggests it's not the problem.

Another option is the interpolation that happens in javascript, going from the native image to screen resolution. I believe it uses nearest neighbour, but I'll have to check. Also I am not sure how this interpolation function deals with transparency (the overlay is complete, but some values are just set to full transparency).

I'll dig into that more

asap.

EDIT: I checked and the weird pattern is present in the sprite of the overlay (I have attached the file for the 1e-6 threshold experiment

). So the problem happens when the sprite is generated in python. Either an interpolation issue, the way I create the alpha mask, or the export in png.

[kchawla-pi]

Hi @pbellec Thanks for doing this!
I noticed that many of the function in the .py files are super long. Consider refactoring them into multiple smaller functions, a ballpark of 7-8 commands per sub-function.
This will simplify debugging, and future maintenance and improvements considerably.
We are starting a push to refactor the code and if new contributions adhere to the practice it will be a huge help.

[pbellec]

I was able to simply use a bootstrap class to get the desired behaviour, and the code is quite concise. I could also implement a pure css style, the code would just be longer

I think I would prefer to write the style rather than use a bootstrap class, in case someone wants to put the brainsprite in a webpage that doesn't use bootstrap

I am having second thoughts about the strategy to resize the iframe.
I think I am going to revert to a fixed width / height for now, and simply adapt these numbers for brainsprite.

In any case we will need different rules for different screen sizes, so let's discuss this in another thread.
I will make a separate PR for scaling iframes once brainsprite is merged.

All I will do in the js_utils is to remove the border of the iframe.

[pbellec]

Hi @pbellec Thanks for doing this!
I noticed that many of the function in the .py files are super long. Consider refactoring them into multiple smaller functions, a ballpark of 7-8 commands per sub-function.
This will simplify debugging, and future maintenance and improvements considerably.
We are starting a push to refactor the code and if new contributions adhere to the practice it will be a huge help.

Yes, I agree. There are several parts of the code that could be naturally splitted. Will work on this.

[pbellec]

why did the jquery version need to change?

It doesn't, but brainsprite somehow broke with the newer version. I reverted back to an old version to open the PR, but I will update and fix the incompatibility asap.

[pbellec]

one other problem with reading image values from the plot is that thresholded
values are displayed as NaN (which is a bit surprising but ok),

An alternative would be to not display the value at all when it is NaN.
either "value = "
or not show value at all.

and values above
vmax are displayed as vmax (which is an error). I guess there is not much we can
do about it.

Well it is strictly showing what the colorbar is showing.
Could add a symbol "-" and "+" after the value when it is vmin or vmax.
If we go down that road, this strictly should also be displayed in the limits of the colorbar as well. That's not done in plot_stat_map.

Maybe we could disable displaying the value at all, since it does
not tell us anything more than what can be read from the plot and the colorbar.

I do appreciate being able to read exact values, especially when showing peaks. Reading from the colorbar is very imprecise. Another consideration in favor of getting rid of the value is that color discretization comes with rounding approximation that some user may pick on: those are not strictly the values you will read in your data array using nibabel, but rather the values you get after spatial resampling and moving to 256 discrete colors. Let me know what you think.

[kchawla-pi]

Once CircleCI passes, I am merging this.
@GaelVaroquaux @jeromedockes @KamalakerDadi

[jeromedockes]

I do appreciate being able to read exact values, especially when showing peaks. Reading from the colorbar is very imprecise. Another consideration in favor of getting rid of the value is that color discretization comes with rounding approximation that some user may pick on: those are not strictly the values you will read in your data array using nibabel, but rather the values you get after spatial resampling and moving to 256 discrete colors. Let me know what you think.

yes as you point out here we are not reading the exact values, we are reading what is shown on the plot + colorbar. so we get the same imprecisions, thresholding and cropping. in particular when showing peaks, we are not reading the peaks' values but only vmax.

If we go down that road, this strictly should also be displayed in the limits of the colorbar as well. That's not done in plot_stat_map.

but it is known that the information we get from the colorbar is a bit imprecise. on the other hand, if we display an actual number, as a user I would expect it to be the true value in the original image, not something that is read from the plot. So I guess I would be in favor of not displaying a value at all. I'm sorry to realize this so late, especially after you added the function to de-duplicate colormaps.

But this is only my opinion, maybe @GaelVaroquaux or @KamalakerDadi will prefer displaying the value anyway.

appart from a few details I'm glad to see this is getting really close to merge!

[jeromedockes]

Once CircleCI passes, I am merging this.

awesome! can we finish the discussion about displaying values first? it will not be a big change either way

[kchawla-pi]

awesome! can we finish the discussion about displaying values first? it will not be a big change either way

I can wait.
If someone pushes something with a change I'll wait for the CI to pass again, no problem.
I will be pushing a brainsprite-cleanup PR to clean up the function renaming and documentation changes. You can tell me what to change and I can include that.

There is also the option of you creating a new PR after this is merged and making the change. I think that will be best.

Lemme know.

[jeromedockes]

There is also the option of you creating a new PR after this is merged and making the change. I think that will be best.

It is likely that we will decide not to change anything at all. but if we decide not to display the value, it would be best if @pbellec does it since he wrote the code.

[pbellec]

There is also the option of you creating a new PR after this is merged and making the change. I think that will be best.

Agreed.

It is likely that we will decide not to change anything at all. but if we decide not to display the value, it would be best if @pbellec does it since he wrote the code.

Happy to contribute to future development as well.

[kchawla-pi]

Cool. @pbellec can do a new PR, while we can merge the rest and remove any merge conflicts that may crop up.
I want tests to run after each merge. Since there are already several PRs that will take a while. I will try to do the release tomorrow morning.

[kchawla-pi]

Any consensus on making the changes or not?

[KamalakerDadi]

one other problem with reading image values from the plot is that thresholded
values are displayed as NaN (which is a bit surprising but ok)

@jeromedockes In this case, you suggest to display the value of underlying image (bg_img) rather than NaN ?

and values above
vmax are displayed as vmax (which is an error). I guess there is not much we can
do about it.

Yes, because vmax is set by the user. By default, we provide vmax=None which displays according to the values of inside the images ?

Anything we could do to improve the current situation about displaying value ?

[jeromedockes]

Any consensus on making the changes or not?

The problem is that there is a design issue with the way brainsprite displays
the "value = ... " field.

We have data, a 3d image, and a first view of it, a plot + colorbar. The view,
as is its role, does not contain all the information in the data. It summarizes
what is in the image and shows the most relevant information in a way that can
easily be understood by the user. In particular, it does not contain the
original image values, but only their image through a colormap, and it does not
contain information about thresholded voxels. The colormap is not injective --
notably, all values above vmax are mapped to the same color, it might have
duplicate values, and the space is binned in at most 256 regions.

Then, as is often done in such interactive viewers, brainsprite wants to offer a
second view of the data, which is complementary to the first one: the "value =
... " field. This second view doesn't allow to grasp the content of the whole
image at once, but offers more precise information about a single voxel at a
time. For example, as @pbellec pointed out, it allows us to see the exact image
values at the peaks, something we cannot read from the plot because they will
often be higher than vmax and mapped to vmax's color.

But in brainsprite, this second view doesn't have access to the data: it tries
to read the information it needs from the first view (the plot). Therefore it
cannot be complementary to the plot or tell the user anything that cannot be
seen on the plot, since the plot is all it has access to. As a result, we have
seen a several problems in practice, which cannot be solved satisfactorily: this
view doesn't work when the cmap contains duplicate values, it cannot show the
true values of the image above vmax or below vmin (which are the most important
ones), it doesn't show the true image values but the inverse image of the colors
through the cmap, it displays NaN for thresholded values.

I don't think this "value = ..." can be made to work correctly unless its design
is changed and it is given the information that it needs.

Moreover, this field is not essential; the viewer is already super useful
without it and even if we leave this feature aside for now, @pbellec has already
achieved the goal of this PR which was to reduce by a factor 10 the memory usage
of nilearn's stat map viewer.

I would therefore suggest we don't display this field for now. If in the future
brainsprite finds a better way to implement this, a PR can be opened to add it.

[jeromedockes]

There is also the option of you creating a new PR after this is merged and making the change. I think that will be best.

I disagree, it makes much more sense to leave it aside and add it when we have a
way to make it work than to introduce a feature that doesn't work and later open
a PR to remove it.

[kchawla-pi]

We are saying the same thing. Deal with this in a separate PR later.

[jeromedockes]

We are saying the same thing. Deal with this in a separate PR later.

No. displaying image values needed to be removed before merging this. then possibly added back in the future (probably not in the near future since it would require important changes in brainsprite).

you can remove it in #1856 with @pbellec 's help.

[pbellec]

@jeromedockes I disagree with the fact the feature is broken. It has the limitations inherent to the visualization presented to the user, no more no less. It is impossible to change the design of brainsprite to address your concern: it would require to store the original data, which is exactly what brainsprite was designed not to do.

In any case, if you think this feature is problematic, I have no problem removing the value and the colormap deduplication. I don't think I can commit in #1856, but I can open a separate PR, which can be merged in #1856 or in master directly.

[bthirion]

I think I'd rather remove the feature for the moment, because it will probably lead to some confusion on the user side.
Sorry, because I know that this is a huge amount of work !

[pbellec]

I think I'd rather remove the feature for the moment, because it will probably lead to some confusion on the user side.
Sorry, because I know that this is a huge amount of work !

Absolutely no problem. It was some work, but useful work. The ability to retrieve values after click is critical to make the viewer interact with other plots. My vision for brainsprite is to become a building block for complex dashboards. I'll move the deduplicate_cmap code to a different code base. It's just not useful for view_stat_map, and that's ok :)

[jeromedockes]

I have no problem removing the value and the colormap deduplication

I would be in favor of this. sorry again for reacting so late on this.

let's repeat the most important point: this PR is a huge win for nilearn, with or without this feature. thanks again @pbellec for this great contribution!

[pbellec]

OK so we have a consensus, will work on this asap.

Now that the big merge has occured (!) I would like to thank you a lot for your patience and sharing your insights @jeromedockes @KamalakerDadi @kchawla-pi and @GaelVaroquaux .

I have learned tons going through this PR thanks to you, and I believe this experience is going to help me a lot producing better code. Best python course ever :)

[KamalakerDadi]

Awesome work by you @pbellec .Congratulations!

[GaelVaroquaux]

Yes, this is an awesome PR, and I am really happy to see this addition to nilearn. We'll be releasing soon, now that this is merged.

…

On Nov 11, 2018, 00:57, at 00:57, KamalakerDadi ***@***.***> wrote: Awesome work by you @pbellec .Congratulations! -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1766 (comment)

[bthirion]

+1 !

[kchawla-pi]

The rename of view_stat_map to view_img is complete. The CI is green.
Thanks @pbellec for this tremendous work you have done!

[jeromedockes]

thanks a lot! great work!