Add composite image config and display options - #222
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCytoDataFrame now renders configurable multi-channel composite crops with color legends, overlays, equalization controls, shared crop geometry, and normalized notebook sizing. Documentation, tests, and the pyproject-fmt hook revision were also updated. ChangesComposite rendering
Pre-commit hook maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CytoDataFrame
participant NotebookRowRenderer
participant process_channel_composite_as_html_display
participant NotebookHTML
CytoDataFrame->>NotebookRowRenderer: Share crop geometry
NotebookRowRenderer->>process_channel_composite_as_html_display: Resolve channels, colors, and equalization
process_channel_composite_as_html_display->>process_channel_composite_as_html_display: Fit, tint, blend, and restore overlays
process_channel_composite_as_html_display-->>NotebookRowRenderer: Return composite HTML image
NotebookRowRenderer->>NotebookHTML: Render composite column and legend
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cytodataframe/frame.py (1)
3504-3510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: log the swallowed exception before returning
"".A failed composite render is silently converted to an empty cell with no diagnostic trail, and Ruff flags the blind
except Exception(BLE001). A debug log keeps the current behavior while making silent-empty composites diagnosable.♻️ Suggested change
try: return self._image_array_to_html(composite_array) - except Exception: + except Exception as exc: + logger.debug("Failed to render composite image as HTML: %s", exc) return ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cytodataframe/frame.py` around lines 3504 - 3510, Update the exception handler around _image_array_to_html to log the caught exception at debug level before returning an empty string. Preserve the existing fallback behavior while replacing the blind exception handling with an explicitly bound exception so Ruff BLE001 is satisfied.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cytodataframe/frame.py`:
- Around line 3095-3101: Normalize numeric width values to CSS pixel strings
before applying the image minimum-width logic in the rendering method containing
_resolve_image_min_width. Ensure configured integer widths such as 300 become
“300px” for both the emitted width style and _resolve_image_min_width, while
preserving existing handling for non-numeric width values.
---
Nitpick comments:
In `@src/cytodataframe/frame.py`:
- Around line 3504-3510: Update the exception handler around
_image_array_to_html to log the caught exception at debug level before returning
an empty string. Preserve the existing fallback behavior while replacing the
blind exception handling with an explicitly bound exception so Ruff BLE001 is
satisfied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af938fa7-6aea-417b-9f8c-a2d409fcd40f
📒 Files selected for processing (6)
.pre-commit-config.yamlREADME.mddocs/src/examples/cytodataframe_at_a_glance.ipynbdocs/src/examples/cytodataframe_at_a_glance.pysrc/cytodataframe/frame.pytests/test_frame.py
|
@d33bs Lucky number 222! |
jenna-tomkinson
left a comment
jenna-tomkinson
left a comment
There was a problem hiding this comment.
Thanks for adding this so quickly! Everything looks good but my main concern is the "all" functionality, as you don't need to call colors. Wouldn't a user need to know what channels are what colors in the composite to interpret the crop if they use this function? I would recommend just a simple print for what colors match which channels, as someone can specify colors with the other functionality.
|
Thanks @jenna-tomkinson for the review! Before I make further changes could you help me address the questions I had based on your review to make sure this aligns with what you expect? |
jenna-tomkinson
left a comment
jenna-tomkinson
left a comment
There was a problem hiding this comment.
This is looking REALLY good but I think we need to make a change to the histogram equalization before we can merge this PR. In the current state, it would be difficult for a user to generate optimal composite images. As well, we should consider either updating or removing the "all" functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/src/examples/cytodataframe_at_a_glance.ipynb (1)
1-2000: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider stripping volatile notebook outputs before committing.
This diff is entirely notebook
outputs: base64-encoded PNG images, widgetmodel_idvalues, and "CPU times"/"Wall time" stdout text. None of these come from cellsourcechanges. Widget IDs and timing text are non-deterministic and will produce diff noise on every re-execution, even without logic changes. Large embedded base64 images also bloat the repository and make the notebook diff hard to review.Strip notebook outputs before committing, for example with
nbstripoutorjupyter nbconvert --clear-output. This PR already updates.pre-commit-config.yaml; add an output-stripping hook there to enforce this automatically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/src/examples/cytodataframe_at_a_glance.ipynb` around lines 1 - 2000, Strip all execution outputs from the notebook, including embedded base64 images, widget model IDs, and timing text, while preserving every cell’s source and metadata needed for the example. Update the existing pre-commit configuration to add an nbstripout or equivalent clear-output hook so future notebook commits enforce this automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cytodataframe/frame.py`:
- Around line 2706-2710: Update the image-cache key in the frame rendering flow
around adjust_with_adaptive_histogram_equalization to include a normalized
equalize_clip_limit from the retained display_options mapping, alongside the
existing path and brightness components. Ensure changing this option on the same
frame produces a cache miss and recomputes pixels, and add a regression test
covering renders before and after modifying equalize_clip_limit.
---
Nitpick comments:
In `@docs/src/examples/cytodataframe_at_a_glance.ipynb`:
- Around line 1-2000: Strip all execution outputs from the notebook, including
embedded base64 images, widget model IDs, and timing text, while preserving
every cell’s source and metadata needed for the example. Update the existing
pre-commit configuration to add an nbstripout or equivalent clear-output hook so
future notebook commits enforce this automatically.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6756db59-5212-4cae-8154-edd2a891589d
📒 Files selected for processing (6)
README.mddocs/src/examples/cytodataframe_at_a_glance.ipynbdocs/src/examples/cytodataframe_at_a_glance.pysrc/cytodataframe/frame.pysrc/cytodataframe/image.pytests/test_frame.py
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
Resolve .pre-commit-config.yaml conflict by taking upstream hook versions (pyproject-fmt v2.26.0, ruff v0.16.1).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cytodataframe/frame.py (2)
3126-3138: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
heightand handle an explicitNonewidth.When
display_options["width"]isNone, Line 3128 passesNonethrough and Line 3131 emitswidth:None. Numeric heights also emit values such asheight:300, which is invalid CSS. Apply the same CSS normalization toheight, and omit or mapNoneto the documented automatic behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cytodataframe/frame.py` around lines 3126 - 3138, Update the display sizing logic around _normalize_css_width so height is normalized to a valid CSS width-style value before adding it to html_style, and handle an explicit None width by omitting the width declaration or using the documented automatic-width behavior instead of emitting width:None. Preserve the existing min-width resolution for valid widths.
5261-5272: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject composite column name collisions.
When
composite_column_namealready exists, Lines 5271-5272 leave the column in place and Line 5284 overwrites its displayed rows. If the name is an image column, Lines 5288-5292 then read composite HTML instead of the original filename. If it is bounding-box or center metadata, crop geometry is corrupted. Reject existing names or generate a unique name before assignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cytodataframe/frame.py` around lines 5261 - 5272, Update the composite-column setup around composite_column_name so an existing data column name is never reused; reject the requested name or generate a unique alternative before assigning composite output. Ensure subsequent rendered-row updates and composite HTML reads use only this collision-free name, preserving image filenames and bounding-box/center metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cytodataframe/frame.py`:
- Around line 3126-3138: Update the display sizing logic around
_normalize_css_width so height is normalized to a valid CSS width-style value
before adding it to html_style, and handle an explicit None width by omitting
the width declaration or using the documented automatic-width behavior instead
of emitting width:None. Preserve the existing min-width resolution for valid
widths.
- Around line 5261-5272: Update the composite-column setup around
composite_column_name so an existing data column name is never reused; reject
the requested name or generate a unique alternative before assigning composite
output. Ensure subsequent rendered-row updates and composite HTML reads use only
this collision-free name, preserving image filenames and bounding-box/center
metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 007ee57d-6fff-48a5-b01f-3ba0d91e8131
📒 Files selected for processing (3)
.pre-commit-config.yamlsrc/cytodataframe/frame.pytests/test_frame.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_frame.py
|
Thanks @jenna-tomkinson - this is ready for a re-review when ready, I think I've addressed all your comments but please let me know if anything's missing. |
jenna-tomkinson
left a comment
jenna-tomkinson
left a comment
There was a problem hiding this comment.
LGTM! I left some more major comments regarding the example notebook and the chosen colors and channels, as these examples are currently non-standard and might not best reflect how the crops will look to someone in reality.
Co-Authored-By: Jenna Tomkinson <107513215+jenna-tomkinson@users.noreply.github.com>
|
Thank you @jenna-tomkinson ! |
2 participants
Description
This adds the ability to view multiple independent single channel images as a composite through CytoDataFrame.
Closes #221
What kind of change(s) are included?
Checklist
Please ensure that all boxes are checked before indicating that this pull request is ready for review.
Summary by CodeRabbit
New Features
Improvements
Documentation