MONAI icon indicating copy to clipboard operation
MONAI copied to clipboard

Fix: add ensure_channel_first in utils and integrate into DiceHelper (refs #8366)

Open mattlin1124 opened this issue 5 months ago • 5 comments

What does this PR do? Adds ensure_channel_first utility in monai/metrics/utils.py and integrates it into DiceHelper.call in monai/metrics/meandice.py to normalize input tensors to channel-first layout before Dice computation. This prevents metric calculation errors when the input is provided in channel-last format.

Issue reference Refs #8366

Motivation and context Currently, DiceHelper assumes the channel dimension is at index 1. If a user passes channel-last tensors (e.g., [N, H, W, C]), it can lead to incorrect indexing and metric values. This PR ensures that y_pred and (when applicable) y are converted to channel-first format before further processing, preserving original metric behavior while adding channel-last support.

How I tested

Verified that channel-last inputs now produce identical results to channel-first inputs.

Ran pytest -q tests/metrics -k dice locally without failures.

Checked that no changes are introduced to the outputs when inputs are already channel-first.

mattlin1124 avatar Aug 09 '25 06:08 mattlin1124

Walkthrough

.gitignore now ignores issue38366/ and issue8366/. Added ensure_channel_first(x: torch.Tensor, spatial_ndim: Optional[int]=None, channel_hint: Optional[int]=None, threshold: int=32) -> Tuple[torch.Tensor, int] in monai/inferers/utils.py to canonicalize tensors to channel-first layout (N, C, spatial...) and return the original channel position (1 if already channel-first, -1 if channel was last). It infers spatial_ndim when omitted, accepts a channel_hint, uses a threshold heuristic to decide layout, reorders with movedim(-1, 1) when needed, and raises on invalid inputs. In monai/metrics/meandice.py a new DiceMetric._compute_tensor(self, y_pred, y) was added; DiceMetric.__call__ and DiceHelper.__call__ now normalize y_pred via ensure_channel_first and conditionally normalize y to channel-first when its trailing dim equals 1 or the number of channels. No public API signatures were removed or renamed.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

[!TIP]

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • [ ] 📝 Generate Docstrings
🧪 Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Aug 09 '25 06:08 coderabbitai[bot]

Hi @mattlin1124 thanks for the contribution but please do look at the Coderabbit complaints to see if there's things to address as well as the CICD fails. The .gitignore file also shouldn't be changed, and we'd need tests for this added behaviour.

I do feel that being a PyTorch library means the running assumption with all data is that it is formatted with channel first. If you have data that's channel last it should be on your to manually account for this, so forcing you to be explicitly aware of what your data is and how you're handling it. Using heuristics as you do here for checking if the channels are first or last is going to lead to false positive cases or hide what is actually being done.

This problem is also present everywhere that we ingest data, that's why we have the EnsureChannelFirst/EnsureChannelFirstd transforms which don't attempt to guess at the input format. If we do want to integrate this heuristic it should be optionally used in places like this as well.

We're planning a release soon which won't include major semantic changes but we will discuss this change for the following release.

ericspod avatar Sep 09 '25 12:09 ericspod

I see your other PR @mattlin1124 here related to sliding window and Dice. This raises the same concerns of course about false positive which I think what Coderabbit is on about.

ericspod avatar Sep 09 '25 13:09 ericspod

Thanks @ericspod for the clear feedback.
I’ll try to add strict shape validation without any implicit guessing.
If the input isn’t channel-first, the function will raise a clear error telling users to apply EnsureChannelFirst/EnsureChannelFirstd upstream.
I’ll also revert the unintended .gitignore change as suggested.

Does this approach sound reasonable?

mattlin1124 avatar Sep 09 '25 15:09 mattlin1124

Thanks @ericspod for the clear feedback. I’ll try to add strict shape validation without any implicit guessing. If the input isn’t channel-first, the function will raise a clear error telling users to apply EnsureChannelFirst/EnsureChannelFirstd upstream. I’ll also revert the unintended .gitignore change as suggested.

Does this approach sound reasonable?

Hi @mattlin1124 this all sounds good. We can review this shortly but we're working on a minor release to go out soon so this will be delayed a bit. Thanks!

ericspod avatar Sep 12 '25 23:09 ericspod