support for multiple slice positions in 2D heatmaps with automatic layout #87

zenWai · 2025-04-01T02:40:17Z

Description

What is this PR

Addition of a new feature

Why is this PR needed?

solves #54

What does this PR do?

Allow position parameter to accept a list of positions in 2D format
Generate plt.Figure with up to 25 positions each in a grid layout.

Example

Is this a breaking change?

No

Does this PR require an update to the documentation?

no

Checklist:

The code has been tested locally
Tests have been added to cover all new functionality (unit & integration)
The documentation has been updated to reflect any changes
The code has been formatted with pre-commit

- Allow position parameter to accept a list of positions in 2D format - Generate plt.Figure with up to 25 positions each in a grid layout. - Add validations to decrease some human error. - Add some assert for mipy, self.slicer depends on position parameter value(s) and can be None

- remove filename from example

codecov · 2025-04-01T02:42:19Z

Codecov Report

Attention: Patch coverage is 15.09434% with 45 lines in your changes missing coverage. Please review.

Project coverage is 63.17%. Comparing base (4e3bf9e) to head (8a2d08c).

Files with missing lines	Patch %	Lines
brainglobe_heatmap/heatmaps.py	15.38%	44 Missing ⚠️
brainglobe_heatmap/planner.py	0.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (4e3bf9e) and HEAD (8a2d08c). Click for more details.

HEAD has 15 uploads less than BASE

Flag BASE (4e3bf9e) HEAD (8a2d08c)

20 5

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
- Coverage   70.03%   63.17%   -6.86%     
==========================================
  Files           5        5              
  Lines         327      372      +45     
==========================================
+ Hits          229      235       +6     
- Misses         98      137      +39

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zenWai · 2025-04-01T03:07:56Z

Ready for review, will leave it as draft for now.
Need special attention to these two aspects I'm unsure about:

The self.slicer initialization as None followed by conditional assignment, this pattern feels a bit awkward but I'm not sure of a better approach.
Using position values as default titles when self.title is None, feels a bit akward and is confusing, at same time I see value on users understanding what they're looking at and a better way to handle all choices does not come to my mind.
Currently self.title=None does default title with positions, self.title="" would have empty title, self.title="title" makes all subplots with given title wich is akward, maybe.
Probably could make self.title give a title for the figure instead?!

Thanks

alessandrofelder

Hey @zenWai

Sorry that other work commitments and Easter holidays have meant it's taken me a long time to look at this.
In general, I like that this PR makes it a lot easier for users to make 2D subplots in a standard way! I've made some nitpicky comments (sorry!) and asked some clarifying questions :)

My two-cents on your questions:

The self.slicer initialization as None followed by conditional assignment, this pattern feels a bit awkward but I'm not sure of a better approach.

An alternative approach I can think of is that in the 2D case, self.slicer is always a list, and we cover the current case by making it a list of length 1 when users pass a single value. That way, we don't need self.multiple_slicers, and internally always make the code iterate over self.slicer - what do you think?

Using position values as default titles when self.title is None, feels a bit akward and is confusing, at same time I see value on users understanding what they're looking at and a better way to handle all choices does not come to my mind.
Currently self.title=None does default title with positions, self.title="" would have empty title, self.title="title" makes all subplots with given title wich is akward, maybe.
Probably could make self.title give a title for the figure instead?!

I don't mind position values as default titles or empty title as is now - I think this is fine.
But 25x the same title is definitely NOT what we want. So I would vote to make self.title be a title for the figure by default? Given that IIUC the PR is backwards compatible, if people want more control over subplot titles they can script that as they could before in the old example?

alessandrofelder · 2025-04-10T17:03:05Z

brainglobe_heatmap/heatmaps.py

@@ -127,6 +127,7 @@ def __init__(
            magnitudes as the values.
        position : list, tuple, np.ndarray, float
            Position of the plane in the atlas.
+            list of positions create multiple slices.


Suggested change

list of positions create multiple slices.

List of positions create multiple slices.

typo.

alessandrofelder · 2025-04-28T15:36:58Z

brainglobe_heatmap/heatmaps.py

+                    "List of positions not supported in 3D format. "
+                    "Did you mean to use a tuple as a 3D position?"
+                )
+            if len(position) <= 1:


I think it should be OK to pass a list of length 1 (but not an empty list)? If you agree, please adapt accordingly?

alessandrofelder · 2025-04-28T15:52:37Z

brainglobe_heatmap/heatmaps.py

@@ -275,6 +297,9 @@ def show(self, **kwargs) -> Union[Scene, plt.Figure]:
        Creates a 2D plot or 3D rendering of the heatmap
        """
        if self.format == "3D":
+            assert (


is there a case where self.slicer is ever still None after running __init__? I don't see it, so not sure this is needed?

alessandrofelder · 2025-04-28T16:18:25Z

examples/heatmap_2d_subplots.py

-# Create a list of scenes to plot
-# Note: it's important to keep reference to the scenes to avoid a
-# segmentation fault
-scenes = []


maybe still worth keeping this example too, for advanced users (like when users want more control over figure size, or more than 25 subplots... or something else) to have more flexibility?

alessandrofelder · 2025-05-30T09:40:34Z

Hey @zenWai - let us know whether you're planning to respond/address my comments (no rush - it took me long enough to even get to review! 😅) or you'd like the core team to take this on (when they find time)

zenWai · 2025-06-23T18:14:08Z

Thanks for the pings, I will revisit it this weekend, I believe I want to

Hey @zenWai
An alternative approach I can think of is that in the 2D case, self.slicer is always a list, and we cover the current case by making it a list of length 1 when users pass a single value. That way, we don't need self.multiple_slicers, and internally always make the code iterate over self.slicer - what do you think?

For sure, this sounds like a good approach, I will implement it and see if I run into any edge issues.

I don't mind position values as default titles or empty title as is now - I think this is fine. But 25x the same title is definitely NOT what we want. So I would vote to make self.title be a title for the figure by default? Given that IIUC the PR is backwards compatible, if people want more control over subplot titles they can script that as they could before in the old example?

Sounds good, I will make the self.title provide a title for the figure would make things easier and makes sense.

I will take a full look this next weekend hopfully have it ready with tests👋

zenWai added 2 commits April 1, 2025 03:30

support for multiple slice positions in 2D heatmaps

8a2d08c

- remove filename from example

adamltyson requested a review from alessandrofelder April 1, 2025 08:40

alessandrofelder requested changes Apr 28, 2025

View reviewed changes

alessandrofelder mentioned this pull request Jun 12, 2025

fix colorbar region labels position according to region values #86

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

support for multiple slice positions in 2D heatmaps with automatic layout #87

support for multiple slice positions in 2D heatmaps with automatic layout #87

Uh oh!

zenWai commented Apr 1, 2025 •

edited

Loading

Uh oh!

codecov bot commented Apr 1, 2025 •

edited

Loading

Uh oh!

zenWai commented Apr 1, 2025

Uh oh!

alessandrofelder left a comment

Uh oh!

alessandrofelder Apr 10, 2025

Uh oh!

alessandrofelder Apr 28, 2025

Uh oh!

alessandrofelder Apr 28, 2025

Uh oh!

alessandrofelder Apr 28, 2025

Uh oh!

alessandrofelder commented May 30, 2025

Uh oh!

zenWai commented Jun 23, 2025

Uh oh!

Uh oh!

	list of positions create multiple slices.
	List of positions create multiple slices.

support for multiple slice positions in 2D heatmaps with automatic layout #87

Are you sure you want to change the base?

support for multiple slice positions in 2D heatmaps with automatic layout #87

Uh oh!

Conversation

zenWai commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Is this a breaking change?

Does this PR require an update to the documentation?

Checklist:

Uh oh!

codecov bot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zenWai commented Apr 1, 2025

Uh oh!

alessandrofelder left a comment

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

alessandrofelder commented May 30, 2025

Uh oh!

zenWai commented Jun 23, 2025

Uh oh!

Uh oh!

zenWai commented Apr 1, 2025 •

edited

Loading

codecov bot commented Apr 1, 2025 •

edited

Loading