-
Notifications
You must be signed in to change notification settings - Fork 1
DEGA-333-transformation-matrix-etc #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
DEGA-333-transformation-matrix-etc #178
Conversation
|
In this PR:
Tested notebooks on Xenium Pancreas public dataset only: ✅ Xenium_pre-process.ipynb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I requested some changes. Once these are done, I can test the notebook on my end. Can you also address the quality/test issues that are showing up?
|
Thank you for the review @cornhundred! All requested changes from both you and Copilot have been implemented. All quality checks and pytests are passing successfully, and the latest changes from main have been merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I requested some changes and made one commit to set the default for image_tile_layer to all - otherwise only the DAPI layer is processed. Later we can change the image_tile_layer to allow a list and we should probably rename it image_tile_layers - but we can do this later.
…f github.com:broadinstitute/celldega into DEGA-333-Add-transformation-matrix-to-LandscapeFiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds transformation matrix functionality to landscape files, enhances the Landscape widget to dynamically fetch technology information, and improves gene handling with deduplication logic.
- Modifies the Landscape widget to dynamically fetch technology from
landscape_parameters.jsonwhen not explicitly provided - Adds transformation matrix support for coordinate transformation in landscape files
- Implements gene deduplication logic to handle duplicate gene names consistently
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/celldega/viz/widget.py | Enhanced Landscape widget with dynamic technology fetching and frontend message handling |
| src/celldega/pre/init.py | Added gene deduplication functions and updated landscape parameter saving with image dimensions |
| src/celldega/pre/run_pre_processing.py | Modified preprocessing pipeline to include gene alignment and image dimension parameters |
| js/widget.js | Added frontend technology fetching logic with fallback handling |
| tests/unit/test_viz/test_widget.py | Updated tests with mocking for technology fetching and syntax fixes |
| tests/unit/test_viz/test_landscape_colors.py | Added mocking for technology fetching in color tests |
| pyproject.toml | Added minimum Python version requirement |
| notebooks/ | Updated notebook examples to remove explicit technology parameter |
|
All feedback from @cornhundred and Copilot has been addressed. Changes from main have been merged, and the working Celldega version is up to date (0.13.0a9). All pytest checks are passing, and ruff formatting and linting are complete. |
…mation-matrix-to-LandscapeFiles
…ght in save_landscape_parameters
|
@cornhundred @huanlity — this PR is ready for review. Summary here: #178 (comment) All prior feedback from @cornhundred and Copilot has been addressed. Latest changes from main have been merged, the working Celldega version is up to date (0.13.0), all pytest checks are passing, and ruff formatting/linting is complete. |
No description provided.