fix(#103): replace mesh projection with direct atlas volume read - #125
fix(#103): replace mesh projection with direct atlas volume read#125ArielShadrac wants to merge 4 commits into
Conversation
dc3102b to
3ba525f
Compare
alessandrofelder
left a comment
There was a problem hiding this comment.
Thanks for your contribution @ArielShadrac
I have managed to make time for an initial look. My general comments are:
- this is quite a big PR, which makes it hard to review: I wonder whether it would be possible to split it up into smaller PR, to make it easier to understand the proposed changes?
- a possible axis reversal is a separate issue compared to using meshes or annotation volumes for visualisation: maybe you could open an issue detailing the axis reversal and how it can be reproduced?
|
Hi @alessandrofelder , thanks for the review and for taking the time on this. You're right on both points. Looking at again the diff more carefully, I can see effectively the PR conflates two independent concerns that should have been separate from the start. Here's my proposed split: PR A (this one, scoped down): The core annotation-volume pipeline get_slice_from_volume, _build_id_to_acronym, build_region_masks_bottomup, the refactored plot_subplot, and _plot_subplot_legacy as a fallback for custom orientations. Unit and integration tests scoped to these functions only. PR B (new): The additional rendering features introduced alongside smooth vectorial contours (__draw_smooth_contours, smooth_contour_path), region labels (_draw_region_labels), and the new Heatmap parameters (color_mode, show_labels, background_color, edge_smooth_sigma, etc.). These are independent of the bug fix and can be reviewed on their own merits. Regarding the axis reversal: I'll open a dedicated issue documenting the root cause (the sign flip on the ML axis in the Slicer/plane.py projection pipeline) with a minimal reproducible example. I'll do that before reworking the PR. Does this split look reasonable to you? Happy to adjust if you'd prefer a different boundary. |
|
Following my previous proposal and after local testing, I need to adjust the boundary slightly. |
f99ab3e to
ab0227d
Compare
|
Hi @alessandrofelder , I've reworked the PR following your feedback. The core annotation-volume pipeline: _get_slice_from_volume, _build_id_to_acronym, _build_region_masks_bottomup, the refactored plot_subplot, and _plot_subplot_legacy as a fallback for custom orientations PR B (to follow) will cover the genuinely optional rendering features: color_mode, show_labels, _draw_region_labels, background_color, and related parameters. |
… Fixes ML-axis inversion and missing colours for parent regions.
bb44b04 to
c5b6e36
Compare
for more information, see https://pre-commit.ci
Closes #103
Problem
The 2D frontal slice was showing only contours with no colours, and
the medio-lateral axis was inverted.
Before (bug #103)
After — correct colours and orientation
Code used to generate the render
```python import brainglobe_heatmap as bghdict1 = {
'mlf': 1.04, 'IO': 0.5, 'IP': 0.40, 'DCO': 0.88, 'VCO': 0.17,
'chpl': 0.14, 'IRN': 0.009, 'V4': 0.023, 'V4r': 0.11, 'PRP': 0.012,
'py': 0.38, 'MV': 0.097, 'SUV': 0.118, 'RO': 0.07, 'SPIV': 0.099,
'RPA': 0.467, 'MARN': 0.010, 'MY': 0.353, 'ICB': 0.056, 'SPVO': 0.036,
'das': 0.101, 'CB': 1.094, 'sctd': 1.318, 'NTS': 0.172, 'arb': 0.022,
'x': 0.112, 'sptV': 0.220, 'DN': 0.054, 'PARN': 0.009, 'rust': 0.042,
'sctv': 0.119, 'LING': 0.056, 'AMBd': 0.0, 'NOD': 0.419, 'PGRNd': 0.178,
'PGRNl': 0.025, 'FN': 0.016, 'PRM': 0.351, 'COPY': 0.267, 'PFL': 0.9,
'tspc': 0.020, 'GRN': 0.024, 'ANcr2': 0.280, 'icp': 0.096, 'VeCB': 0.053,
}
bgh.Heatmap(
dict1,
position=11500,
orientation="frontal",
thickness=100,
vmin=min(dict1.values()),
vmax=max(dict1.values()),
format="2D",
).show()