-
Notifications
You must be signed in to change notification settings - Fork 1
Post-BTEP Presentation Updates #152
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
Conversation
Get codebase working using the new Maestro environment management sys…
…t broke a published dashboard
Unsupervised clustering: changed aspect ratios and added normalization options for the plots
…workspaces and published dashboards, taken from the TAC deployment
… probably from IMC deployment
fix scatter plot lag
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 contains updates following the BTEP presentation that adjust plotting performance and appearance in several modules, update package installation preferences, and remove obsolete configuration files.
- Updated plotting functions to use Scattergl and refined aesthetic parameters in Pheno Cluster plots.
- Introduced new normalization toggles and adjusted marker sizes in UMAP and scatter plots.
- Modified package installer preference and updated channel checks; removed the legacy pages.toml configuration.
Reviewed Changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pages2/robust_scatter_plotter.py | Switched to Scattergl for improved performance in scatter plotting. |
| pages2/Pheno_Cluster_b.py | Updated plotting layout, normalization options, and marker sizes. |
| pages2/Pheno_Cluster_a.py | Adjusted plotly layout settings to ensure consistent aspect ratios. |
| install_missing_packages.py | Removed 'conda' from the list of installers to use. |
| app_top_of_page.py & Multiplex_Analysis_Web_Apps.py | Updated channel checking logic. |
| .streamlit/pages.toml | Removed configuration file likely reflecting a reorganization. |
| .envs/maestro/meta.yaml | Added metadata and package dependency definitions. |
Files not reviewed (3)
- .hooks/startup.sh: Language not supported
- environment.yml: Language not supported
- templateConfig.json: Language not supported
Comments suppressed due to low confidence (4)
.streamlit/pages.toml:1
- [nitpick] The removal of the pages.toml file suggests a change in navigation configuration; please confirm that this removal aligns with the updated app navigation design.
Removed file content
pages2/Pheno_Cluster_b.py:127
- [nitpick] The new figure size setting is significantly smaller than before; please confirm that this change improves visualization and does not adversely affect readability.
plotnine.options.figure_size = (5, 4)
pages2/Pheno_Cluster_b.py:156
- [nitpick] The marker size has been reduced from 8 to 3 in the scatterplot; please verify that the smaller markers maintain adequate visibility across different displays.
palette='viridis', ax=ax, s=3)
install_missing_packages.py:71
- Removal of 'conda' from the installers may affect environments where conda is preferred; please ensure that this change is intentional and consistent with deployment practices.
installers_to_use = ['mamba', 'pip']
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
Updates following the BTEP presentation to bump template versions, improve plot performance and styling, add normalization toggles to the Phenotype Clustering pipeline, and consolidate environment/install scripts.
- Bumped parent template version in
templateConfig.json - Swapped
go.Scattertogo.Scatterglfor faster scatter rendering - Added normalization options, aspect‐ratio fixes, and font size adjustments in Pheno_Cluster
- Removed old environment.yml and revised installer scripts
- Updated startup hook and platform detection logic
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| templateConfig.json | Bump parentTemplateVersion from 0.176.0 to 0.192.0. |
| pages2/robust_scatter_plotter.py | Swapped Scatter to Scattergl for WebGL‐based rendering. |
| pages2/Pheno_Cluster_b.py | Added normalization toggles, sizing tweaks, and aspect‐ratio fixes; updated function signature. |
| pages2/Pheno_Cluster_a.py | Applied aspect‐ratio fix to UMAP plots. |
| install_missing_packages.py | Removed conda fallback from installer list. |
| environment.yml | Deleted entire file (migrating to meta.yaml). |
| app_top_of_page.py & Multiplex_Analysis_Web_Apps.py | Switched platform check from "nidap.nih.gov" to "foundry-artifacts". |
| .streamlit/pages.toml | Entire file removed. |
| .hooks/startup.sh | Uninstalled pip/mamba install commands. |
| .envs/maestro/meta.yaml | Introduced new consolidated conda/pip recipe. |
Comments suppressed due to low confidence (6)
.hooks/startup.sh:9
- All essential pip and mamba install commands were removed, which may prevent required dependencies from being installed during startup; verify that startup.sh still installs necessary packages.
pip install scikit-learn streamlit-extras squidpy split-file-reader st-pages dill pympler objsize
.streamlit/pages.toml:1
- The pages.toml file has been completely removed, which will break custom Streamlit page navigation; confirm that page configuration is intentionally migrated or updated.
[[pages]]
pages2/Pheno_Cluster_b.py:98
- The 'plot_column' parameter is no longer used in the function body; consider removing it or integrating its use to avoid confusion.
def phenocluster__plot_diff_intensity_2(adata, groups, method, n_genes, plot_column, normalize_total, log_normalize, z_normalize, cluster_group):
pages2/Pheno_Cluster_b.py:98
- [nitpick] The function name uses a double underscore which can be confusing and violates typical Python naming conventions; consider renaming to use a single underscore or a more descriptive name.
def phenocluster__plot_diff_intensity_2(adata, groups, method, n_genes, plot_column, normalize_total, log_normalize, z_normalize, cluster_group):
environment.yml:1
- The 'environment.yml' file has been deleted; ensure that its removal is intentional and that environment management is consolidated in meta.yaml.
- -dependencies:
install_missing_packages.py:71
- Removing 'conda' from installers_to_use removes a fallback option if mamba isn't available; consider re-adding 'conda' as a fallback installer.
installers_to_use = ['mamba', 'pip']
| Check if the Streamlit application is operating on NIDAP | ||
| ''' | ||
| return np.any(['nidap.nih.gov' in x for x in subprocess.run('conda config --show channels', shell=True, capture_output=True).stdout.decode().split('\n')[1:-1]]) | ||
| return np.any(['foundry-artifacts' in x for x in subprocess.run('conda config --show channels', shell=True, capture_output=True).stdout.decode().split('\n')[1:-1]]) |
Copilot
AI
May 31, 2025
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.
The docstring for 'platform_is_nidap' still refers to checking for NIDAP, but the logic checks for 'foundry-artifacts'; update the docstring to reflect the current check.
… per-image and per-dataset basis in the robust scatter plotter as toggles after the scatter plot is displayed.
Get value counts and percentages on per-image and per-dataset basis
Changing False to Left and True to Right
Incidence Figure Red Boxes + Threshold Pheno
Co-authored-by: Copilot <[email protected]>
Feature creation
To better view splittable features
Missed one of the checks
So we get an idea of the level of unique values for a given column
To correct for existing archives
andrew-weisman
left a comment
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.
Approved as long as the bugs from the 7/29/25 meeting are addressed so we're sure this is a fully functional version. Is that the case @djsmith17? Thanks for taking the initiative.
Just to avoid the inability to perform the clustering
Co-authored-by: Copilot <[email protected]>
Unique value fix
Tracking new updates to MAWA following the BTEP presentation