Skip to content

Reorder layers in adaptive mask plot - #1480

Merged
tsalo merged 1 commit into
ME-ICA:mainfrom
tsalo:fix-adaptive-mask-plot
Jul 22, 2026
Merged

tsalo merged 1 commit into
ME-ICA:mainfrom
tsalo:fix-adaptive-mask-plot

Conversation

@tsalo

@tsalo tsalo commented Jul 20, 2026

Copy link
Copy Markdown
Member

Closes none, but spins off a small bugfix from #1478.

Changes proposed in this pull request:

  • Reverse order of mask lines in plot_adaptive_mask so that initial mask is on the bottom and the classification mask is on the top.

@tsalo tsalo added the reports issues related to boilerplate generation or visual reports label Jul 20, 2026
@tsalo

tsalo commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

Here are the figures from the five-echo test. You'll be able to see that the majority of the initial mask is retained in the final mask, and the new plot correctly shows a blue line (Classification, OC & Initial) for that mask instead of a red line (Initial mask only).

Before

adaptive_mask_before

After

adaptive_mask_after

@tsalo tsalo added the bug issues describing a bug or error found in the project label Jul 20, 2026
@tsalo
tsalo requested a review from handwerkerd July 20, 2026 15:55
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.43%. Comparing base (fd18a66) to head (edc89aa).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1480   +/-   ##
=======================================
  Coverage   90.43%   90.43%           
=======================================
  Files          31       31           
  Lines        5100     5100           
  Branches      937      937           
=======================================
  Hits         4612     4612           
  Misses        326      326           
  Partials      162      162           

☔ View full report in Codecov by Harness.
📢 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.

@eurunuela eurunuela left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! Thanks!

@tsalo
tsalo merged commit 13b78cb into ME-ICA:main Jul 22, 2026
16 checks passed
@tsalo
tsalo deleted the fix-adaptive-mask-plot branch July 22, 2026 02:43
@handwerkerd

handwerkerd commented Jul 23, 2026

Copy link
Copy Markdown
Member

@tsalo I was traveling, but I wanted to mention we did originally compare the two color contours (see : #1333 (comment) )

The reason I prefer the initial mask as the top layer is because that makes the user-provided mask very clear and highlights any deviations from the initial mask.

That is, if you look at the following, it's clear that a non-trivial number of voxels in the user-provided mask were not included in the ICA mask.
image

For the flipped version, there's a little dot of magenta but it seems to be a bit less clear how much is lost.
image

If others really like the now-current flipped ordering, I won't push back more than this.

@tsalo

tsalo commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

I can revert the change, but we should fix the labels. The "initial mask only" label is accurate with the new order because any time you see magenta it means that that bit corresponds to voxels that were only present in the initial mask. In the old version that label wasn't accurate, since it reflected the initial mask, but not only the initial mask.

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

Labels

bug issues describing a bug or error found in the project reports issues related to boilerplate generation or visual reports

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants