netneurolab / netneurotools

Useful tools from the Network Neuroscience Lab
https://netneurolab.github.io/netneurotools
BSD 3-Clause "New" or "Revised" License
57 stars 33 forks source link

Add custom_aspect parameter to plot_point_brain. #104

Closed VinceBaz closed 3 years ago

VinceBaz commented 3 years ago

The function plot_point_brain() sets the limits of the xyz axes with the following lines of code:

ax.set(xlim=0.57 * np.array(ax.get_xlim()), ylim=0.57 * np.array(ax.get_ylim()), zlim=0.60 * np.array(ax.get_zlim()))

It also zscores the coordinates beforehand.

This might be problematic since these limits, being hard-coded, can lead to various bugs. For instance, if we use the coordinates of the Lausanne 1000 parcellation, some parcels are cropped out:

image

To solve this particular bug, I modified those lines to

ax.set(xlim=0.665 * np.array(ax.get_xlim()), ylim=0.665 * np.array(ax.get_ylim()), zlim=0.70 * np.array(ax.get_zlim())).

We now get this when we try to plot the Lausanne 1000 parcellation:

image.

The parcels are no longer cropped out.

Also, I personally enjoy visualizing my data such that the axes are automatically scaled to have "equal" aspect ratios, so I added a custom_aspect parameter to the function that can be set to False if the user wants. If this parameter is set to False, then the axes will be automatically scaled, which gives us:

image.

This comes with the caveat that there is more space between the differerent views, and now the 3 brains are not perfectly aligned on top of one another. Also, the sagittal view is now much more elongated, which might be dissapointing if the user wants to plot a "toy" version of the brain that looks nice. So for this reason, this custom_aspect parameter is set to True by default (which gives us what we had before).

liuzhenqi77 commented 3 years ago

Hi Vince, I think since last year, plot_point_brain were not displaying the right perspective/shape, and these fixed numbers (to set the limit) might be for the previously-working version. Ross @rmarkello has made some changes #68 to remove the aspect parameter, but at the time it still wouldn't work nicely because there was a bug in matplotlib/Axes3D (see matplotlib/matplotlib#1077, matplotlib/matplotlib#8894, matplotlib/matplotlib#17172), and there were quite some effects to fix it (see matplotlib/matplotlib#8896, matplotlib/matplotlib#16472, matplotlib/matplotlib#17515).

Now it seems 3d aspect is fixed in 3.3.0 using set_box_aspect from matplotlib/matplotlib#17515, we could maybe work a bit to fully fix the point brain that we all need!

It also seems aspect="equal" (matplotlib/matplotlib#1077) is still not fixed until now.

VinceBaz commented 3 years ago

Ohh Thanks @liuzhenqi77. I did remember that Ross modified the code because at some point, they decided to remove the implementation of ax.set_aspect('equal') altogether: https://github.com/matplotlib/matplotlib/pull/13474. It turns out that replacing the line

ax.auto_scale_xyz(*[[np.min(scaling), np.max(scaling)]] * 3)

to ax.set_box_aspect(tuple(scaling[:, 1] - scaling[:, 0]))

makes our brains look better: we scale the box aspect (as the name of the function suggests) rather than directly set the limits of the axes. So basically, we get from this (the modified version I proposed above; I added the axes so that we can see what is happening):

image

to this:

image

I think that scaling the box aspect is better. And for some reason, the sagittal view now looks much better (I also added a parameter that allows us to choose whether we want our different views to be horizontally aligned or vertically aligned)

VinceBaz commented 3 years ago

To confirm that the set_box_aspect() function works as expected, I looked at what the aspect would look like, for the sagittal view, in 2d space, with aspect='equal':

image.

It looks exactly the same. Since the three views are now much better, I think that we should always want to set the aspect ratio to equal. So I updated my pull request and got rid of the "custom_aspect" parameter and as I mentionned in my previous comment, I added a views_orientation parameter as well.

codecov-commenter commented 3 years ago

Codecov Report

Merging #104 (18fab34) into master (46fdb31) will decrease coverage by 0.06%. The diff coverage is 77.77%.

:exclamation: Current head 18fab34 differs from pull request most recent head 55b1a51. Consider uploading reports for the commit 55b1a51 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   72.05%   71.99%   -0.07%     
==========================================
  Files          26       26              
  Lines        1893     1896       +3     
==========================================
+ Hits         1364     1365       +1     
- Misses        529      531       +2     
Impacted Files Coverage Δ
netneurotools/plotting.py 65.18% <77.77%> (-0.36%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 46fdb31...55b1a51. Read the comment docs.

rmarkello commented 3 years ago

This is awesome! :tada: :raised_hands: Thanks @VinceBaz for the PR and @liuzhenqi77 for digging through the myriad matplotlib issues + errors + updates.

I'm happy to take a look at the code but since @liuzhenqi77 was here first maybe you want to take lead on the review? :grin: I've gone ahead and assigned it to you but let me know if you'd prefer not to.

liuzhenqi77 commented 3 years ago

Looks great to me! Pinging @rmarkello