[ENH] Improve plotting contours for `PlotlySurfaceFigure` objects by adding `add_contours` method #3949

psadil · 2023-09-04T23:35:53Z

Closes [ENH] improving plot_surf_contours #3711

Changes proposed in this pull request:

Add add_contours method to PlotlySurfaceFigure, increasing customizability of contour lines.
Added example of the method in plot_3d_map_to_surface_projection

github-actions · 2023-09-04T23:36:10Z

👋 @psadil Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

PR has an interpretable title.
PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
Code is PEP8-compliant (see our documentation on coding style)
Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

There is at least one unit test per new function / class (see our documentation on testing)
The new feature is demoed in at least one relevant example.

For bug fixes:

There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

codecov · 2023-09-04T23:50:00Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6578329) 91.77% compared to head (cb7f960) 91.80%.
Report is 190 commits behind head on main.

Files	Patch %	Lines
nilearn/plotting/displays/_figures.py	94.33%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3949      +/-   ##
==========================================
+ Coverage   91.77%   91.80%   +0.03%     
==========================================
  Files         134      134              
  Lines       15751    15822      +71     
  Branches     3283     3298      +15     
==========================================
+ Hits        14455    14526      +71     
  Misses        752      752              
  Partials      544      544

Flag	Coverage Δ
macos-latest_3.10	`91.72% <94.33%> (+0.03%)`	⬆️
macos-latest_3.11	`91.72% <94.33%> (+0.03%)`	⬆️
macos-latest_3.8	`91.68% <94.33%> (+0.03%)`	⬆️
macos-latest_3.9	`91.68% <94.33%> (+0.03%)`	⬆️
ubuntu-latest_3.10	`91.72% <94.33%> (+0.03%)`	⬆️
ubuntu-latest_3.11	`91.72% <94.33%> (+0.03%)`	⬆️
ubuntu-latest_3.8	`91.68% <94.33%> (+0.03%)`	⬆️
ubuntu-latest_3.9	`?`
windows-latest_3.10	`91.66% <94.33%> (+0.03%)`	⬆️
windows-latest_3.11	`91.66% <94.33%> (+0.03%)`	⬆️
windows-latest_3.8	`91.62% <94.33%> (+0.03%)`	⬆️
windows-latest_3.9	`91.62% <94.33%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ymzayek · 2023-09-05T14:50:25Z

@psadil thanks for getting started on this! Will try to start a review this week

nilearn/plotting/displays/_figures.py

psadil · 2023-09-06T03:26:10Z

Tests added!

Do these seem like they're on the right track?

nilearn/plotting/displays/_figures.py

Remi-Gau · 2023-09-06T13:57:37Z

Tests added!

Thanks!!

Do these seem like they're on the right track?

Had a very quick look: it does help also understand how these methods are used, so definitely useful

nilearn/plotting/displays/_figures.py

Co-authored-by: Remi Gau <remi_gau@hotmail.com>

nilearn/plotting/displays/_figures.py

nilearn/plotting/tests/test_surf_plotting.py

ymzayek · 2023-09-07T07:22:45Z

Thanks @psadil for the updates! Here is the built example: https://output.circle-artifacts.com/output/job/9e7e0100-bdac-4eaf-99d5-b5d2f512e8bd/artifacts/0/dev/auto_examples/01_plotting/plot_3d_map_to_surface_projection.html#sphx-glr-auto-examples-01-plotting-plot-3d-map-to-surface-projection-py

It looks good! There is a small skip that I see in the postcentral gyrus contour. Idk if that can be improved further. And the other contour is hard to see but I guess that is to showcase the line width feature.

Also the API reference looks good: https://output.circle-artifacts.com/output/job/9e7e0100-bdac-4eaf-99d5-b5d2f512e8bd/artifacts/0/dev/modules/generated/nilearn.plotting.displays.PlotlySurfaceFigure.html#nilearn.plotting.displays.PlotlySurfaceFigure

ymzayek · 2023-09-07T07:34:01Z

BTW while checking this I noticed that in the same example where plot_surf_contours is used the contours are not rendered anymore. This is not related to this PR as I can also see it in the build artifacts dating back at least a month. I'll open a separate issue

psadil · 2023-09-07T12:38:44Z

It looks good! There is a small skip that I see in the postcentral gyrus contour. Idk if that can be improved further. And the other contour is hard to see but I guess that is to showcase the line width feature.

Oh dear, thanks. I think this might be a rendering issue. But let me find a way to either confirm or fix. Aiming for the next couple of days.

bthirion

LGTM besides the last glitch mentioned by @ymzayek

ymzayek

Mostly small doc stuff. I think this is in a good enough state and we can merge after addressing these. A follow-up PR can be done to address the small skip to close the contour. I don't think it should block this

ymzayek · 2023-09-14T09:34:06Z

examples/01_plotting/plot_3d_map_to_surface_projection.py

@@ -134,11 +134,15 @@
 ##############################################################################
 # Display outlines of the regions of interest on top of a statistical map
 # -----------------------------------------------------------------------
+#
+# Regions can be outlined using both engines.


Maybe add a line clarifying that for the default matplotlib engine used by plot_surf_stat_map, plotting.plot_surf_contours is used.

Makes me also think we should raise an error in plot_surf_contours if plotly object is passed. Now we get the error AttributeError: 'PlotlySurfaceFigure' object has no attribute 'axes' but it would be better to raise one specifying that plot_surf_contours only works with matplotlib. WDYT?

yes a more explicit error message would be a good thing for users

ymzayek · 2023-09-14T09:47:13Z

examples/01_plotting/plot_3d_map_to_surface_projection.py

+# Note that the contours are added with a method of the object that is
+# returned by :func:`~nilearn.plotting.plot_surf_stat_map`:
+# :meth:`~nilearn.plotting.displays.PlotlySurfaceFigure.add_contours`.


Suggested change

# Note that the contours are added with a method of the object that is

# returned by :func:`~nilearn.plotting.plot_surf_stat_map`:

# :meth:`~nilearn.plotting.displays.PlotlySurfaceFigure.add_contours`.

# Note that the contours are added with

# :meth:`~nilearn.plotting.displays.PlotlySurfaceFigure.add_contours`

# method of a :class:`~nilearn.plotting.displays.PlotlySurfaceFigure` object

# that is returned by :func:`~nilearn.plotting.plot_surf_stat_map`

# when engine is set to "plotly".

ymzayek · 2023-09-14T10:00:53Z

nilearn/plotting/displays/_figures.py

+        """
+        Draw boundaries around roi.


Just for consistency

Suggested change

"""

Draw boundaries around roi.

"""Draw boundaries around roi.

ymzayek · 2023-09-14T10:03:21Z

nilearn/plotting/displays/_figures.py

+            The value at each vertex one inside the ROI and zero inside ROI,
+            or an :obj:`int` giving the label number for atlases.


??

Suggested change

The value at each vertex one inside the ROI and zero inside ROI,

or an :obj:`int` giving the label number for atlases.

The value at each vertex is one inside the ROI and zero outside

the ROI, or an :obj:`int` giving the label number for atlases.

ymzayek · 2023-09-14T10:07:25Z

nilearn/plotting/displays/_figures.py

+        """
+        Identify which vertices lie on the outer edge of a parcellation.


Suggested change

"""

Identify which vertices lie on the outer edge of a parcellation.

"""Identify which vertices lie on the outer edge of a parcellation.

ymzayek · 2023-09-14T10:18:30Z

nilearn/plotting/displays/_figures.py

+
+        Returns
+        -------
+        data : :class:`numpy.ndarray`


Suggested change

data : :class:`numpy.ndarray`

sorted_vertices : :class:`numpy.ndarray`

ymzayek · 2023-09-14T12:25:33Z

Oh can you also add a whatsnew entry in doc/changes/latest.rst?

psadil · 2023-09-17T17:34:51Z

A follow-up PR can be done to address the small skip to close the contour. I don't think it should block this

Just to give an update, I'm now suspecting a bug, though I'm not sure of the source. Plotting the sorted_vertices with markers shows that one vertex was missing from the postcentral gyrus contour. This vertex that is not in the contour ends up in the second-to-last row of sorted_vertices array, and the skip seems to be related to how the lines are drawn when trying to account for this other vertex (one edge of the gap is row in sorted_vertices that is one before the one that is missing from the contour.)

So, I'm hoping to sort this out before the pull request is merged.

ymzayek · 2023-09-18T07:54:01Z

I see in that case we'll hold off until that is resolved. Thanks @psadil

psadil · 2023-09-28T16:08:20Z

Here's a wireframe view of the surface to help clarify the situation:

Vertices in the roi are marked with circles. The circles are orange when they are on the edge of the boundary (defined as a vertex of a triangle inside the roi and on a face with only two vertices in the roi) and otherwise purple.

Building up the boundary edge by sequentially selecting the next closest vertex ends up skipping the orange vertex that is lowest in the figure (the one with only one orange neighbor).

Is there an expectation for how to trace this boundary? I see these options

Color every edge between boundary vertices, and only those edges (would lead to faces whose edges are only partially colored)
Color every edge between boundary vertices, and only those edges, but exclude vertices that have only one neighbor on the graph (some vertices would be missing from an roi)
Color the outer edges of every face that has at least two vertices in the roi (could lead to the boundaries of two rois overlapping)

ymzayek · 2023-10-02T09:59:36Z

@psadil first thought would be to go with the third option since it seems most conservative, however the visual consequence of overlapping boundaries is not clear. We'd need to test it.

psadil · 2023-10-08T00:51:21Z

Sorry, but hoping to check another implementation detail. I wonder how to handle drawing handle regions that do not share an edge with another vertex in the roi. For example, this region has 2 isolated vertices.

Should I follow the matplotlib approach? I think that would mean ignoring the isolated vertices.

ymzayek · 2023-10-09T08:20:08Z

@psadil thanks for checking that. I think then it would be best to stay consistent with the matplotlib approach and ignore isolated vertices. That would then be more like the 2nd option you describe in #3949 (comment)?

psadil · 2023-10-09T16:18:00Z

@psadil thanks for checking that. I think then it would be best to stay consistent with the matplotlib approach and ignore isolated vertices. That would then be more like the 2nd option you describe in #3949 (comment)?

It wouldn't quite be the 2nd option, because it looks like the current matplotlib approach colors the faces around vertices that share only one edge with another vertex in the ROI. For example, here is the post-central gyrus contour in matplotlib

In the plotly implementation, I think that a comparable boundary could be the one traced by the purple line in the following (the vertex with only one shared edge is included in the contour)

bthirion · 2023-10-09T20:27:21Z

I don't know if there are conventions somewhere that explain what is best. If there aren't I would go for the easiest and most maintainable solution (and possibly a warning if some vertices have to be missed).

ymzayek · 2023-10-10T08:03:55Z

I see, thanks @psadil for the clear explanation. There are certainly pros and cons to each approach so if we make it clear what the contours include in the docstring and consider @bthirion 's last comment I think we can move forward with one of these implementations.

psadil · 2023-10-10T13:31:18Z

Thanks for all the input. I'll work on something that tries to match the matplotlib approach (unless someone encounters another convention in the meanwhile).

FWIW, the behavior of plot_surf_roi includes isolated vertices. But hopefully a clear explanation of how the contours are generated (and warnings about when an isolated vertex is excluded) will avoid too much confusion.

ymzayek · 2023-11-10T08:51:23Z

@psadil would you have the bandwidth to finish this in the next month or two? It would be great to include it in the major release we are planning for January. Thanks for all your work on this!

psadil added 2 commits September 4, 2023 19:27

allow specification of linewidth in contours drawn with plotly

a573742

remove extra arg from docstring

ca73877

psadil added 2 commits September 4, 2023 22:26

shorten line

1c9d024

run flake8 on example

fc89686

Remi-Gau reviewed Sep 5, 2023

View reviewed changes

nilearn/plotting/displays/_figures.py Show resolved Hide resolved

Remi-Gau reviewed Sep 5, 2023

View reviewed changes

nilearn/plotting/displays/_figures.py Show resolved Hide resolved

ymzayek changed the title ~~Improving plot surf contours~~ [ENH] Improve plotting contours for PlotlySurfaceFigure objects by adding add_contours method Sep 5, 2023

add tests for new methods

4739ca0

psadil added 3 commits September 5, 2023 23:35

delay import to avoid always requiring plotly

8e793fb

add test for missed method

e395ba2

fix cross-reference to plotly attribute

78bdfda

Remi-Gau reviewed Sep 6, 2023

View reviewed changes

nilearn/plotting/displays/_figures.py Outdated Show resolved Hide resolved

Remi-Gau reviewed Sep 6, 2023

View reviewed changes

nilearn/plotting/displays/_figures.py Outdated Show resolved Hide resolved

Remi-Gau reviewed Sep 6, 2023

View reviewed changes

nilearn/plotting/displays/_figures.py Outdated Show resolved Hide resolved

Remi-Gau reviewed Sep 6, 2023

View reviewed changes

nilearn/plotting/displays/_figures.py Outdated Show resolved Hide resolved

Remi-Gau reviewed Sep 6, 2023

View reviewed changes

nilearn/plotting/displays/_figures.py Outdated Show resolved Hide resolved

psadil and others added 4 commits September 6, 2023 10:08

Update nilearn/plotting/displays/_figures.py

9753884

Co-authored-by: Remi Gau <remi_gau@hotmail.com>

Update nilearn/plotting/displays/_figures.py

ef4edf2

Co-authored-by: Remi Gau <remi_gau@hotmail.com>

Update nilearn/plotting/displays/_figures.py

b41cd16

Co-authored-by: Remi Gau <remi_gau@hotmail.com>

reference plotly in intersphinx

b681315

ymzayek reviewed Sep 6, 2023

View reviewed changes

nilearn/plotting/displays/_figures.py Outdated Show resolved Hide resolved

nilearn/plotting/displays/_figures.py Outdated Show resolved Hide resolved

nilearn/plotting/tests/test_surf_plotting.py Outdated Show resolved Hide resolved

psadil added 4 commits September 6, 2023 12:23

import plotly at more central location, eliminating repeated imports

e3fc2bb

add missing cross-references

9d12271

add docstring to new tests

c256ed9

fix spelling

84f9885

separate parameter definitions and descriptions

cb7f960

ymzayek mentioned this pull request Sep 7, 2023

[BUG] Contours in plot_3d_map_to_surface_projection examples are not being rendered correctly #3954

Closed

bthirion approved these changes Sep 13, 2023

View reviewed changes

Remi-Gau mentioned this pull request Sep 13, 2023

extract is_plotly_installed function #3974

Closed

ymzayek reviewed Sep 14, 2023

View reviewed changes

Remi-Gau added Surface Related to surface data or surface analysis. Plotting The issue is related to plotting functionalities. labels Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Improve plotting contours for `PlotlySurfaceFigure` objects by adding `add_contours` method #3949

[ENH] Improve plotting contours for `PlotlySurfaceFigure` objects by adding `add_contours` method #3949

psadil commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

codecov bot commented Sep 4, 2023 •

edited

ymzayek commented Sep 5, 2023

psadil commented Sep 6, 2023 •

edited

Remi-Gau commented Sep 6, 2023

ymzayek commented Sep 7, 2023

ymzayek commented Sep 7, 2023

psadil commented Sep 7, 2023

bthirion left a comment

ymzayek left a comment

ymzayek Sep 14, 2023

ymzayek Sep 14, 2023

Remi-Gau Sep 14, 2023

ymzayek Sep 14, 2023

ymzayek Sep 14, 2023

ymzayek Sep 14, 2023

ymzayek Sep 14, 2023

ymzayek Sep 14, 2023

ymzayek commented Sep 14, 2023

psadil commented Sep 17, 2023

ymzayek commented Sep 18, 2023

psadil commented Sep 28, 2023 •

edited

ymzayek commented Oct 2, 2023

psadil commented Oct 8, 2023

ymzayek commented Oct 9, 2023

psadil commented Oct 9, 2023

bthirion commented Oct 9, 2023

ymzayek commented Oct 10, 2023

psadil commented Oct 10, 2023

ymzayek commented Nov 10, 2023

	"""
	Draw boundaries around roi.
	"""Draw boundaries around roi.

		The value at each vertex one inside the ROI and zero inside ROI,
		or an :obj:`int` giving the label number for atlases.

		"""
		Identify which vertices lie on the outer edge of a parcellation.

	"""
	Identify which vertices lie on the outer edge of a parcellation.
	"""Identify which vertices lie on the outer edge of a parcellation.

	data : :class:`numpy.ndarray`
	sorted_vertices : :class:`numpy.ndarray`

[ENH] Improve plotting contours for PlotlySurfaceFigure objects by adding add_contours method #3949

Are you sure you want to change the base?

[ENH] Improve plotting contours for PlotlySurfaceFigure objects by adding add_contours method #3949

Conversation

psadil commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

codecov bot commented Sep 4, 2023 • edited

Codecov Report

ymzayek commented Sep 5, 2023

psadil commented Sep 6, 2023 • edited

Remi-Gau commented Sep 6, 2023

ymzayek commented Sep 7, 2023

ymzayek commented Sep 7, 2023

psadil commented Sep 7, 2023

bthirion left a comment

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymzayek commented Sep 14, 2023

psadil commented Sep 17, 2023

ymzayek commented Sep 18, 2023

psadil commented Sep 28, 2023 • edited

ymzayek commented Oct 2, 2023

psadil commented Oct 8, 2023

ymzayek commented Oct 9, 2023

psadil commented Oct 9, 2023

bthirion commented Oct 9, 2023

ymzayek commented Oct 10, 2023

psadil commented Oct 10, 2023

ymzayek commented Nov 10, 2023

[ENH] Improve plotting contours for `PlotlySurfaceFigure` objects by adding `add_contours` method #3949

[ENH] Improve plotting contours for `PlotlySurfaceFigure` objects by adding `add_contours` method #3949

codecov bot commented Sep 4, 2023 •

edited

psadil commented Sep 6, 2023 •

edited

psadil commented Sep 28, 2023 •

edited