-
Notifications
You must be signed in to change notification settings - Fork 575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] Fix aspect ratio in surface plotting #2899
[FIX] Fix aspect ratio in surface plotting #2899
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2899 +/- ##
==========================================
- Coverage 88.63% 88.61% -0.02%
==========================================
Files 101 101
Lines 13818 13830 +12
Branches 2688 2690 +2
==========================================
+ Hits 12247 12255 +8
- Misses 979 983 +4
Partials 592 592 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least, the fix for plot_surf
seems good.
dffc04d
to
ea0f01e
Compare
Update: ea0f01e proposes a possible fix for import hcp_utils as hcp
from nilearn import plotting, surface
surf = surface.load_surf_mesh(hcp.mesh.flat)
plotting.view_surf(surf) You should get: With the problem of the initial zoom that still need to be fixed. That is, initially the figure looks like this: and you have to zoom out. |
I still have the problem with the initial zoom. |
Weird... 🤔 |
LGTM now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Maybe a small whatsnew entry ?
@@ -76,22 +76,22 @@ function getCamera(plotDivId, viewSelectId) { | |||
} | |||
} | |||
let cameras = { | |||
"left": {eye: {x: -1.7, y: 0, z: 0}, | |||
"left": {eye: {x: -3.0, y: 0, z: 0}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is that now the brain is too small and too far from the colorbar when plotting a surface map on a mesh in mni space (or close to it), which is the vast majority of use cases:
now:
I don't think deteriorating the plots for the most common use case to address an edge case is the way to go. maybe we should allow users to control the zoom (although they can do it with the mouse once the interactive plot is opened, and maybe further programmatic customization of the plots should be deferred to after #2902 is merged)
otherwise the fix to control the aspect ratio looks good!
I can take this one over. It is still an issue and concluding from what is done here I would keep the aspect ratio fix for both |
|
Fixes #2658
WIP...
Example
For
plot_surf
:Now gives:
TODO: Fix
view_surf
which, as pointed out by @jeromedockes, suffers from the same problem although it relies on a different machinery.