Closed
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the factor match score (FMS) functionality by extracting FMS calculations and plotting helpers from individual figure scripts into common modules, while also updating dataset import routines and dependencies.
- Updated Python requirement and bumped core dependencies in
pyproject.toml. - Reorganized
pf2rnaseq/imports.pyand added a newprepare_dataset_deviancefunction. - Moved FMS logic into
factorization.pyand added generic FMS plotting incommonFuncs/plotGeneral.py; cleaned up figure scripts.
Reviewed Changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Bumped Python version and upgraded dependency versions |
| pf2rnaseq/imports.py | Cleaned imports; added prepare_dataset_deviance; updated import functions |
| pf2rnaseq/gating.py | Removed obsolete gating definitions |
| pf2rnaseq/figures/figureHeiserR2XDev.py | New deviance-based R2X figure |
| pf2rnaseq/figures/figureHeiserR2X.py | Adjusted rank ranges and removed unused code |
| pf2rnaseq/figures/figureHeiserGenesDev.py | New deviance-driven gene factors figure |
| pf2rnaseq/figures/figureHeiserGenes.py | Switched to precomputed factor file for gene plotting |
| pf2rnaseq/figures/figureHeiserGenePac.py | Added gene PaCMAP figure |
| pf2rnaseq/figures/figureHeiserFactors.py | Added consolidated factors plotting from stored file |
| pf2rnaseq/figures/figureHeiserFMSDev.py | Separated deviance FMS plots |
| pf2rnaseq/figures/figureHeiserFMS.py | Refactored FMS plotting and removed inline utility functions |
| pf2rnaseq/figures/figureHeiserDevianceFactors.py | Updated subplot sizing and deviance PF2 rank |
| pf2rnaseq/figures/figureHeiserCorrelation.py | Minor import reorder |
| pf2rnaseq/figures/figureHeiserCorrFactors.py | Added metadata extraction helpers for correlation factors |
| pf2rnaseq/figures/figureHeiserCondPac.py | Updated condition PaCMAP source and label type |
| pf2rnaseq/figures/figureHeiserCompPac.py | Updated dataset source and expanded component range |
| pf2rnaseq/figures/figureHeiserCellTypePer.py | Streamlined imports |
| pf2rnaseq/figures/figureHeiserCellPac.py | Streamlined imports |
| pf2rnaseq/figures/figureCytokine20.py | Cleaned formatting in gene‐expression plotting |
| pf2rnaseq/figures/figureCITEseq4.py | Removed obsolete CITEseq4 figure |
| pf2rnaseq/figures/figureCITEseq3.py | Removed obsolete CITEseq3 figure |
| pf2rnaseq/figures/figureCITEseq2.py | Removed obsolete CITEseq2 figure |
| pf2rnaseq/figures/figureAveGene.py | Cleaned imports and formatting |
| pf2rnaseq/figures/commonFuncs/plotPaCMAP.py | Added pre-check for missing gene and early exit |
| pf2rnaseq/figures/commonFuncs/plotGeneral.py | Introduced generic FMS plot helpers |
| pf2rnaseq/factorization.py | Extracted fms_percent_drop and fms_diff_ranks into standalone functions |
Comments suppressed due to low confidence (2)
pf2rnaseq/imports.py:16
- [nitpick] Parameter
geneThresholduses mixed casing. For consistency with Python conventions, use snake_case (e.g.,gene_threshold).
def prepare_dataset_deviance(
pf2rnaseq/figures/figureHeiserCondPac.py:1
- [nitpick] This bare string is not a proper module docstring. Wrap it in triple quotes (
"""Plot condition pacmap""") so it’s recognized as documentation.
"Plot condition pacmap"
| runs=3, | ||
| ): | ||
| """Plots FMS when using different Pf2 components""" | ||
| df = plot_fms_diff_ranks(X, ranksList, runs) |
There was a problem hiding this comment.
This function calls itself recursively, leading to infinite recursion. It should call the standalone helper (e.g., fms_diff_ranks) instead of plot_fms_diff_ranks.
Comment on lines
+6
to
+7
| from pathlib import Path | ||
|
|
There was a problem hiding this comment.
Duplicate import of Path from pathlib. Consider removing redundant imports to keep the module clean.
Suggested change
| from pathlib import Path |
|
|
||
| ranks = list(range(1, 31)) | ||
| ranks = list(range(10, 101, 10)) | ||
| plot_r2x(X, ranks, ax[0]) |
There was a problem hiding this comment.
Both R2X plots are drawn on the same axis ax[0]. Likely unintended overlap; consider using separate subplots or clearing the axis before the second plot.
Comment on lines
1
to
4
| """ | ||
| factorization score | ||
| factorization score | ||
| """ |
There was a problem hiding this comment.
[nitpick] The docstring duplicates the description on two lines. Consider consolidating to one clear docstring line.
Suggested change
| """ | |
| factorization score | |
| factorization score | |
| """ | |
| """factorization score""" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updating FMS-moving out of figure file and plotting functions