Skip to content
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 slicer pre-transforming the planes' center #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

carlocastoldi
Copy link
Contributor

The Slicer automatically set the centre x positioning to its negative value.
This, however, is not something whose responsibility is of the constructor, but rather of the renderer.
As a consequence, it required some weird problems when messing with slicers' planes.

Additionally, it adds the parameter both to plan.show().

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

How has this PR been tested?

from brainrender import Atlas, settings

import brainglobe_heatmap as bgh
import numpy as np

settings.SHOW_AXES = True

atlas = Atlas(atlas_name="allen_mouse_25um")
center = np.array([100, 100, 100])
slicer = bgh.slicer.Slicer(center, "frontal", 10, atlas.get_region("root"))
assert all(slicer.plane0.center == center)

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

Copy link

codecov bot commented Jan 19, 2025

Codecov Report

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

Project coverage is 0.00%. Comparing base (aa723a8) to head (756680f).

Files with missing lines Patch % Lines
brainglobe_heatmap/planner.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main     #71   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          5       5           
  Lines        284     283    -1     
=====================================
+ Misses       284     283    -1     

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

* this created problems when adding a plane mesh into a scene
* additionally it adds the parameter 'both' to plan.show()
Copy link
Member

@IgorTatarnikov IgorTatarnikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great thank you! Unfortunately, removing the plane transformation breaks the sagittal orientation slicing in both the 2D and 3D heatmaps.

I believe this issue will have to be resolved in brainrender and will address #72 and #55 at the same time.

@@ -51,7 +51,6 @@ def __init__(
)

position = np.array(position)
position[2] = -position[2]
Copy link
Member

@IgorTatarnikov IgorTatarnikov Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is required for sagittal slicing to work correctly. There are some underlying oddities in how brainrender handles the last spatial dimension (x in zyx representation). Without this line sagittal slices in 2D look like:

2d_heatmap_wrong_plane

This was generated by running the heatmap_2d.py example specifying the sagittal orientation. It should look something like:

2d_heamap_sagittal

I believe this is tied together with #72 and #55. I think getting rid of this line will require changes in brainrender.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants