Skip to content

Fix 2D morphology coloring to use per-compartment values#60

Merged
mstimberg merged 6 commits intobrian-team:masterfrom
Akshat1000Sharma:fix-2d-morphology-compartment-coloring
Mar 31, 2026
Merged

Fix 2D morphology coloring to use per-compartment values#60
mstimberg merged 6 commits intobrian-team:masterfrom
Akshat1000Sharma:fix-2d-morphology-compartment-coloring

Conversation

@Akshat1000Sharma
Copy link
Copy Markdown
Contributor

@Akshat1000Sharma Akshat1000Sharma commented Mar 1, 2026

Fixed #54

Previously, 2D morphology plotting applied a single color per section, effectively using only the first compartment’s mapped value. This caused incorrect visualization when values differed across compartments.

In _plot_morphology2D, colors are now computed and applied per compartment.

  • For show_diameter=False: each compartment is drawn as an individual segment with its own color.
  • For show_diameter=True: each compartment is drawn as its own polygon patch with compartment-specific diameters and color.
  • Scalar-like values are still supported by repeating one value across the section to preserve intentional single-color behavior.
  • 3D plotting logic remains unchanged.

Added a regression test to verify per-compartment color differentiation.

@Akshat1000Sharma
Fix 2D morphology coloring to use per-compartment values
f7c60d5 
Previously, 2D morphology plotting applied a single color per section,
effectively using only the first compartment’s mapped value. This caused
incorrect visualization when values differed across compartments.

In _plot_morphology2D, colors are now computed and applied per compartment.

- For show_diameter=False: each compartment is drawn as an individual segment with its own color.
- For show_diameter=True: each compartment is drawn as its own polygon patch with compartment-specific diameters and color.
- Scalar-like values are still supported by repeating one value across the section to preserve intentional single-color behavior.
- 3D plotting logic remains unchanged.

Added a regression test to verify per-compartment color differentiation.

All morphology plotting tests pass.
@mstimberg
Copy link
Copy Markdown
Member

mstimberg commented Mar 17, 2026

Hi @Akshat1000Sharma Thank you for the PR, and apologies for having taken so long to reply. I did a first quick review, and I think it looks good 👍
Could you please merge the latest master into this branch? I'd also suggest changing two things:

  1. replace the except Exception by a more specific type (TypeError?)
  2. inline the _section_colors code – I find it a bit confusing to have this internal function that is only used once.

Akshat1000Sharma and others added 2 commits March 18, 2026 10:13
@Akshat1000Sharma
Merge branch 'master' into fix-2d-morphology-compartment-coloring
609aaa3
@Akshat1000Sharma
Inline section color logic and replace broad exception handling
cb7fc90 
- Replaced broad `except Exception` with more specific exception handling
- Inlined `_section_colors` function into `_plot_morphology2D`
- Preserved existing behavior for both scalar and per-compartment values
@Akshat1000Sharma
Copy link
Copy Markdown
Contributor Author

Akshat1000Sharma commented Mar 18, 2026

Thanks @mstimberg for your time and valuable feedback. I have incorporated the suggested changes:

  1. Replaced the broad except Exception with a more specific exception.
  2. Inlined the _section_colors logic directly into _plot_morphology2D while preserving the original behavior.

I have also merged the latest master into this branch.
Please let me know if any further adjustments are needed.

@Akshat1000Sharma
Copy link
Copy Markdown
Contributor Author

Akshat1000Sharma commented Mar 18, 2026

Hi @mstimberg, would you like me to open a separate issue and submit another PR to address the failing test cases? I have reviewed the code and believe I can work on fixing them.

@mstimberg
Copy link
Copy Markdown
Member

mstimberg commented Mar 19, 2026

Hi @Akshat1000Sharma, in principle I'd be happy for you to open a PR, but @ulekarsiddhant0-boop also had started working on it (see #68) – not sure whether he's still interested in opening a PR?

@mstimberg
Copy link
Copy Markdown
Member

mstimberg commented Mar 27, 2026

Hi @Akshat1000Sharma, could you please fix the merge conflict with most recent master? The test suite on master shouldn't be failing anymore.

@Akshat1000Sharma
Copy link
Copy Markdown
Contributor Author

Akshat1000Sharma commented Mar 29, 2026 •
edited
Loading

Thank you for your message @mstimberg. I have updated my branch with most recent master branch. Please let me know if anything else is needed.

@mstimberg
Copy link
Copy Markdown
Member

mstimberg commented Mar 30, 2026

Thanks, the PR itself looks good, but it seems as if the merge was not completely correct – the "FIXME" lines that were removed by #56 shouldn't be necessary anymore when using add_patch instead of add_artist.

@Akshat1000Sharma
Fix incorrect merge by restoring add_patch usage and removing obsolet…
a21bf43 
…e autoscaling workaround

This commit fixes an issue introduced during the merge with upstream where
`axes.add_artist()` was mistakenly used instead of `axes.add_patch()` in
`_plot_morphology2D`.

Using `add_artist()` prevented matplotlib from including the polygon in axis
autoscaling, which required an additional invisible plotting workaround
(`axes.plot(..., alpha=0.)`). This workaround is no longer necessary.

Changes:
- Replaced `axes.add_artist(patch)` with `axes.add_patch(patch)`
- Removed obsolete FIXME comment and autoscaling workaround line

The fix restores correct autoscaling behavior while preserving the
per-compartment coloring logic introduced in this branch.
@Akshat1000Sharma
Copy link
Copy Markdown
Contributor Author

Akshat1000Sharma commented Mar 30, 2026

Hi @mstimberg, thanks for pointing that out.
You were right, the merge wasn’t fully correct on my end. I had merged upstream changes at the Git level, but I missed aligning some of the logic, which is why the leftover FIXME lines were still present.
I’ve now corrected this by properly incorporating the upstream changes and removing the outdated workaround.
Could you please take another look at the updated version when you get a chance?

mstimberg
mstimberg reviewed Mar 31, 2026
View reviewed changes
Comment thread brian2tools/plotting/morphology.py Outdated
@Akshat1000Sharma
Fix: Remove dead code in _plot_morphology2D
caba489 
Remove the defensive ndim == 1 check on compartment_colors,
which is unreachable since voltage_colormap always receives an array
and returns a 2D (N, 4) RGBA result.
@Akshat1000Sharma
Copy link
Copy Markdown
Contributor Author

Akshat1000Sharma commented Mar 31, 2026

Hi @mstimberg,
Since all the discussions and issues on this PR have been addressed, I would greatly appreciate it if you could take a moment to review and merge it. It has been open for quite some time now.
Please let me know if any further changes are needed or if you have any questions. I’d be happy to make updates.

@mstimberg mstimberg merged commit 217879c into brian-team:master Mar 31, 2026
1 check passed
@ulekarsiddhant0-boop
Copy link
Copy Markdown

ulekarsiddhant0-boop commented Apr 6, 2026 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mstimberg mstimberg mstimberg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2D Morphology plot does not plot values per compartment

3 participants

@Akshat1000Sharma @mstimberg @ulekarsiddhant0-boop