Skip to content

Conversation

iyassou
Copy link

@iyassou iyassou commented Sep 1, 2025

Description

I noticed when attempting to extract the hematoxylin stain from an H&E stained image that the previous heuristic for ordering hematoxylin first and eosin second incorrectly assumed hematoxylin has a higher red channel value, when the opposite is typically true. I've corrected this and implemented a more robust heuristic that compares red-blue ratios instead, since:

  • Hematoxylin (nuclear, blue): lower red/blue ratio
  • Eosin (cytoplasm, pink): higher red/blue ratio

I've updated the tests to reflect the corrected stain ordering, and the output now accurately reflects the documented behaviour (first column hematoxylin, second eosin).

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.

The previous heuristic incorrectly assumed hematoxylin has higher red channel values than eosin, when the opposite is typically true.

Replace red-channel-only comparison with more robust red-blue ratio comparison to better distinguish hematoxylin from eosin stains based on their spectral properties.

- Hematoxylin (nuclear, blue): lower red/blue ratio -> first column
- Eosin (cytoplasm, pink): higher red/blue ratio -> second column

Update tests to reflect corrected stain ordering. Documented behaviour (first column = H, second = E) now matches the actual output.

Signed-off-by: Iyassou Shimels <[email protected]>
Copy link

coderabbitai bot commented Sep 1, 2025

Walkthrough

The HE stain ordering logic in monai/apps/pathology/transforms/stain/array.py now uses a red-to-blue ratio comparison between two candidate vectors (v_min and v_max), with an epsilon to avoid division by zero. If v_min’s ratio is lower than v_max’s, the order is kept (hematoxylin first); otherwise, vectors are swapped. Comments were added to explain the rationale. Corresponding tests in tests/apps/pathology/transforms/test_pathology_he_stain.py and test_pathology_he_stain_dict.py were updated to reflect the swapped stain column orientation and resulting normalized image values. No public API signatures changed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

✨ 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 or @coderabbit 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
monai/apps/pathology/transforms/stain/array.py (1)

88-96: Use ASCII name for epsilon and make the ratio robust to sign/near-zero blue; stabilize ties.

Avoid Unicode identifiers, and guard against negative/near-zero denominators and noisy ties. This reduces mis-ordering under sign flips and tiny B.

-        # Hematoxylin: high blue, lower red (low R/B ratio)
-        # Eosin: high red, lower blue (high R/B ratio)
-        ε = np.finfo(np.float32).eps
-        v_min_rb_ratio = v_min[0, 0] / (v_min[2, 0] + ε)
-        v_max_rb_ratio = v_max[0, 0] / (v_max[2, 0] + ε)
-        if v_min_rb_ratio < v_max_rb_ratio:
+        # Hematoxylin: high blue, lower red (low |R|/|B| ratio)
+        # Eosin: high red, lower blue (high |R|/|B| ratio)
+        eps = np.finfo(np.float32).eps
+        v_min_rb_ratio = np.abs(v_min[0, 0]) / (np.abs(v_min[2, 0]) + eps)
+        v_max_rb_ratio = np.abs(v_max[0, 0]) / (np.abs(v_max[2, 0]) + eps)
+        # keep order on ties for determinism
+        if v_min_rb_ratio <= v_max_rb_ratio:
             he = np.array((v_min[:, 0], v_max[:, 0]), dtype=np.float32).T
         else:
             he = np.array((v_max[:, 0], v_min[:, 0]), dtype=np.float32).T
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between d4ba52e and c30d291.

📒 Files selected for processing (3)
  • monai/apps/pathology/transforms/stain/array.py (1 hunks)
  • tests/apps/pathology/transforms/test_pathology_he_stain.py (4 hunks)
  • tests/apps/pathology/transforms/test_pathology_he_stain_dict.py (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

  • monai/apps/pathology/transforms/stain/array.py
  • tests/apps/pathology/transforms/test_pathology_he_stain.py
  • tests/apps/pathology/transforms/test_pathology_he_stain_dict.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
  • GitHub Check: build-docs
  • GitHub Check: quick-py3 (windows-latest)
  • GitHub Check: quick-py3 (ubuntu-latest)
  • GitHub Check: flake8-py3 (codeformat)
  • GitHub Check: packaging
  • GitHub Check: quick-py3 (macOS-latest)
  • GitHub Check: flake8-py3 (pytype)
  • GitHub Check: flake8-py3 (mypy)
  • GitHub Check: min-dep-py3 (3.10)
  • GitHub Check: min-dep-py3 (3.11)
  • GitHub Check: min-dep-pytorch (2.8.0)
  • GitHub Check: min-dep-py3 (3.12)
  • GitHub Check: min-dep-os (ubuntu-latest)
  • GitHub Check: min-dep-os (windows-latest)
  • GitHub Check: min-dep-pytorch (2.6.0)
  • GitHub Check: min-dep-py3 (3.9)
  • GitHub Check: min-dep-pytorch (2.7.1)
  • GitHub Check: min-dep-pytorch (2.5.1)
  • GitHub Check: min-dep-os (macOS-latest)
🔇 Additional comments (7)
tests/apps/pathology/transforms/test_pathology_he_stain_dict.py (3)

42-46: Expected stain matrix update matches the new H/E ordering heuristic.

Numbers reflect lower |R|/|B| for H (first col). Good.


61-66: Normalized image expectation updated consistently with swapped columns.

Looks correct given the new he ordering.


201-214: Docstring aligns with the new derivation and values.

Narrative and matrices are consistent.

tests/apps/pathology/transforms/test_pathology_he_stain.py (4)

48-52: Expected stain matrix reflects H first (lower |R|/|B|).

Matches the revised heuristic.


67-72: Normalized image expectation updated for the new column order.

Consistent with extraction change.


136-139: Docstring example adjusted to the new ordering.

Values and explanation are coherent.


207-219: Case 4 docstring and matrices updated correctly.

Concentration reordering and final image values track the new H/E order.

@ericspod ericspod requested a review from bhashemian September 11, 2025 23:49
@ericspod
Copy link
Member

Hi @bhashemian would you be able to review this change or suggest who might be better placed? Thanks!

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