Skip to content

Updating FMS-moving out of figure file and plotting functions#17

Merged
aarmey merged 1 commit intomainfrom
Moving-FMS-Calc
Jun 28, 2025
Merged

Updating FMS-moving out of figure file and plotting functions#17
aarmey merged 1 commit intomainfrom
Moving-FMS-Calc

Conversation

@nbedanova
Copy link
Contributor

Moved FMS calculation to factorization file and plotting to plotGeneral file.

@nbedanova nbedanova requested a review from Copilot June 27, 2025 21:54
Copy link
Contributor

Copilot AI left a 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 refactors the FMS-related code by moving FMS calculation from the figure file into the factorization file and centralizing the plotting functions into a common plotting module. Key changes include:

  • Activating the plot_fms_percent_drop function in the figure module with an added rank parameter.
  • Adjusting the range of component indices passed to the plotting function.
  • Extracting and re-implementing FMS calculation and plotting functions within the factorization and commonFuncs modules.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
pf2rnaseq/figures/figureHeiserFMS.py Removed redundant functions and updated plotting call parameters.
pf2rnaseq/figures/commonFuncs/plotGeneral.py Introduced centralized plotting functions for FMS.
pf2rnaseq/factorization.py Added FMS calculation and new plotting utility functions.
Comments suppressed due to low confidence (2)

pf2rnaseq/figures/figureHeiserFMS.py:24

  • The change in the rank range from [30, 51) to [1, 31) represents a significant alteration in behavior. Please verify that this updated range aligns with the intended component indices for the FMS figures.
    ranks = list(range(1, 31))

pf2rnaseq/factorization.py:43

  • The removal of the 'regParam' parameter from the pf2 function changes its interface. Confirm that all dependent code has been updated accordingly and that this change is in line with the intended design.
    tolerance=1e-9,

@nbedanova nbedanova requested a review from aarmey June 27, 2025 21:55
Copy link
Member

@aarmey aarmey left a comment

Choose a reason for hiding this comment

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

Fantastic.

@aarmey aarmey merged commit 77661f8 into main Jun 28, 2025
1 check passed
@aarmey aarmey deleted the Moving-FMS-Calc branch June 28, 2025 02:04
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