Add ax arg to plot method

[RobertoDF]

• Bug fix
• Addition of a new feature
• Other

I would like to be able to do something like this:

fig, axs = plt.subplots(1,3, figsize=(15,4))
for ax, d in zip(axs,[8800, 9300, 9700]):
    scene = bgh.Heatmap(
        regions,
        position=(d),
        orientation="frontal",  # or 'sagittal', or 'horizontal' or a tuple (x,y,z)
        thickness=10,
        title="frontal",
        format="2D",
    )
    scene.plot(ax=ax)

Is there a better way to achieve this?

Thanks!

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

[alessandrofelder]

Thanks for your contribution @RobertoDF - I think in theory I am happy to add this functionality!

I have some worry about breaking backwards compatibility (see my comment).

I can also not reproduce the example script you gave in the PR description (I got a segmentation fault when I tried 🤔 ). Could you post a full version of that, including import statements, and how you define regions so I can manually test, please?

[alessandrofelder]

This breaks backwards compatibility right?
Could this be made to still behave as before (unless you pass the new arguments), please?

[RobertoDF]

sure, thanks for the quick answer.

[RobertoDF]

import matplotlib.pyplot as plt
import numpy as np
import brainglobe_heatmap as bgh

data_dict={
 'VISpm': 0.0,
 'VISp': 0.14285714285714285,
 'VISl': 0.2857142857142857,
 'VISli': 0.42857142857142855,
  'VISal': 0.5714285714285714,
 'VISrl': 0.7142857142857142,
 'SSp-bfd': 0.8571428571428571,
 'VISam': 1.0
}
fig, axs = plt.subplots(3,2, figsize=(18,12))
show_cbar = False

for ax, d in zip(axs.flatten(),range(7500,10500,500)):

    scene = bgh.Heatmap(
        data_dict,
        position=(d),
        orientation="frontal",  # or 'sagittal', or 'horizontal' or a tuple (x,y,z)
        thickness=10,
        format="2D",
        cmap="Set2",
        vmin=0,
        vmax=1,
        label_regions=True
    )
    scene.plot(ax=ax, show_cbar=show_cbar, hide_axes=True)

plt.tight_layout()
plt.show()

Does this work?

[alessandrofelder]

Does this work?

I can see the figure in a fresh conda environment with Python 3.10, but not in fresh conda environments with Python 3.11 or 3.12. In all cases, I get a segmentation fault (after closing the figure in 3.10).

[alessandrofelder]

I am on Ubuntu 22.04

[RobertoDF]

Even more weird: if i define the plotting function outside jupyter (so I import it with from Utils.Utils import plot_brainrender_heatmaps) everything works perfectly without crashing.

[IgorTatarnikov]

Hey all! I ran into the same seg fault on Ubuntu. A small change in the example script fixed it for me:

import matplotlib.pyplot as plt
import numpy as np
import brainglobe_heatmap as bgh

data_dict={
 'VISpm': 0.0,
 'VISp': 0.14285714285714285,
 'VISl': 0.2857142857142857,
 'VISli': 0.42857142857142855,
  'VISal': 0.5714285714285714,
 'VISrl': 0.7142857142857142,
 'SSp-bfd': 0.8571428571428571,
 'VISam': 1.0
}
show_cbar = True

fig, axs = plt.subplots(3, 2, figsize=(18, 12))
scenes = []

for ax, d in zip(axs.flatten(),range(7500,10500,500)):
    scene = bgh.Heatmap(
        data_dict,
        position=(d),
        orientation="frontal",  # or 'sagittal', or 'horizontal' or a tuple (x,y,z)
        thickness=10,
        format="2D",
        cmap="Reds",
        vmin=0,
        vmax=1,
        label_regions=False
    )
    scene.plot_subplot(fig=fig, ax=ax, show_cbar=show_cbar, hide_axes=False)
    scenes.append(scene)

plt.tight_layout()
plt.show()

My best guess is that matplotlib does something lazily when plotting, and the garbage collector was being really eager and freeing the memory for each scene before plt.show() was called. Keeping a reference to each scene active fixed the seg fault for me!

I'm not sure if this is something we should address within the implementation or if it's best to publish this example script somewhere in the docs as the "proper" way of doing it.

[alessandrofelder]

Do you have a sense of how easy it would be to address this in the implementation?

[IgorTatarnikov]

I think this could be addressed by somehow changing Heatmap to allow storing multiple slicers internally, that way instead of this being done in a for loop you'd initialize one scene with multiple 2D positions. Then the plot() function can have internal logic to check if multiple slicers are present and provide a figure with a subplot for each slicer.

[IgorTatarnikov]

Just realised I didn't actually answer your question! In terms of ease I'd say it wouldn't be too bad, just a bit more refactoring. The main thing I'd worry about is adding too many conditionals, or how to expose this to the user without overloading the position argument in the __init__ further!

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (8d45490) to head (e8610a2).
Report is 16 commits behind head on main.

Files	Patch %	Lines
brainglobe_heatmap/heatmaps.py	0.00%	13 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main     #44   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          5       5           
  Lines        279     284    +5     
=====================================
- Misses       279     284    +5     

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

[alessandrofelder]

Thanks a lot @IgorTatarnikov
The changes are sensible, and thanks for adding more comprehensive docstrings.
I've also checked that I can run the example locally.

Can I suggest that you

• open an issue about how this could be done properly
• add the example to the examples/ subfolder, with a comment in the code about how it's important to keep the references to the scenes around.

and then merge, please? 🙏

[IgorTatarnikov]

Done and done! I think this is good to merge.