Keep all modseq mode for ProSIMSIt #31

fhamood · 2025-05-20T14:47:13Z

No description provided.

…lus in case its a decoy

…with a plus in case its a decoy" This reverts commit 75124a9.

This reverts commit 85978da.

…string of an integer is added to a float series

…to delete a table that does not exist

… from start

Copilot

Pull Request Overview

Introduce a new keep_all ambiguity mode to capture all PSMs in clusters, refine related utilities, and update argument handling and parsing.

Added list_all helper and expanded transfer to support keep_all with optional overwrite behavior
Refined type hints in utils, updated CLI help, and ensured scanID is cast to int in TMT processing
Tweaked cleanup in main, corrected docstring in evidence.py, and broadened pyarrow version in pyproject.toml

Reviewed Changes

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

Show a summary per file

File	Description
simsi_transfer/utils/utils.py	Adjusted type hints, removed unused imports, and added `list_all`
simsi_transfer/transfer.py	Added `keep_all` mode, `overwrite` flag, new aggregation logic
simsi_transfer/tmt_processing.py	Cast `scanID` to `int` for both MS2 and MS3 extraction
simsi_transfer/main.py	Left TODO comment, changed variable deletion order
simsi_transfer/evidence.py	Updated docstring params to reflect current signature
simsi_transfer/command_line_interface.py	Enhanced help for `--ambiguity_decision` with new mode
pyproject.toml	Expanded `pyarrow` version constraint

Comments suppressed due to low confidence (3)

simsi_transfer/command_line_interface.py:107

Consider adding choices=['majority', 'all', 'keep_all'] to this argument to enforce valid inputs and catch typos early.

apars.add_argument('--ambiguity_decision', default='majority', metavar="S",

simsi_transfer/utils/utils.py:69

The type hint no longer accepts Set[Any] even though the implementation handles sets. Restore Union[List[Any], Set[Any]] to keep the signature accurate.

def remove_nan_values(input_list: List[Any]) -> List[Any]:

simsi_transfer/tmt_processing.py:171

The indentation and parentheses look misaligned for the int(re.search(...)[1]) call. Verify proper indentation and balanced parentheses to prevent syntax errors.

scanseries["scanID"] = int(re.search(r"scan=(\d+)", item["id"])[1])

simsi_transfer/utils/utils.py

Copilot · 2025-05-20T14:53:03Z

simsi_transfer/utils/utils.py

    return ";".join(sorted(set([x for x in s.split(";") if len(x) > 0])))


+def list_all(x: pd.Series):


The new list_all utility lacks a return type annotation and docstring. Consider adding -> List[Any] and a brief description for consistency.

Suggested change

def list_all(x: pd.Series):

def list_all(x: pd.Series) -> List[Any]:

"""

Converts a pandas Series to a list.

:param x: A pandas Series object.

:return: A list containing the elements of the Series.

"""

Copilot · 2025-05-20T14:53:04Z

simsi_transfer/transfer.py



-def transfer(summary_df, max_pep=False, mask=False, ambiguity_decision='majority'):
+def transfer(summary_df, max_pep=False, mask=False, ambiguity_decision='majority', overwrite=False):


The new overwrite parameter isn’t exposed in the CLI or documented. Add a corresponding flag or update the documentation so users can enable it.

Copilot · 2025-05-20T14:53:04Z

simsi_transfer/transfer.py

+        summary_df.loc[:, replacement_dict.keys()] = summary_df.loc[:, replacement_dict.values()]
+    else:
        # Aren't all of these cells by definition NaNs? Can we make use of that?
        # Like 'Merge, and if you find two columns with the same name always use the value that is not NaN!'
        # Then we could do this directly in the merge and wouldn't have to go over the df again here
        # Try pd.DataFrame.combine(). For this the dfs would need to be in the same shape though.
-        summary_df[identification_column] == 't', replacement_dict.keys()] = summary_df.loc[
-        summary_df[identification_column] == 't', replacement_dict.values()].to_numpy()
+        summary_df = summary_df.astype({col: 'object' for col in replacement_dict.keys()})
+        for col in replacement_dict.keys():


Using replacement_dict.keys() returns a dict_keys object, which may not work in loc. Convert to a list or explicit column list to avoid indexing errors.

simsi_transfer/main.py

Copilot · 2025-05-20T14:53:05Z

simsi_transfer/main.py


-        del msms_simsi, evidence_simsi
+        del msms_simsi



[nitpick] Previously both msms_simsi and evidence_simsi were deleted together; now only one is freed here and the other earlier. Review cleanup logic to ensure consistent memory release.

Copilot · 2025-05-20T14:53:05Z

simsi_transfer/evidence.py

+    :param tempfile: Temporary processing file, e.g., MaRaCluster clustering dataframe
+    :param msms_df: MaxQuant msms.txt dataframe
+    :return: Merged dataframe


The docstring parameters no longer match the function signature. Update or remove entries to reflect actual arguments (tempfile, msms_df, etc.).

Suggested change

:param tempfile: Temporary processing file, e.g., MaRaCluster clustering dataframe

:param msms_df: MaxQuant msms.txt dataframe

:return: Merged dataframe

:param summary: Summary dataframe containing information about MS2 scans.

:param evidence: Evidence dataframe containing precursor information.

:param allpeptides: AllPeptides dataframe containing peptide-level information.

:param plex: Integer specifying the plex level for the experiment.

:param num_threads: Number of threads to use for parallel processing.

:return: A concatenated dataframe containing merged evidence information.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Anonoemus added 15 commits July 12, 2024 14:36

added option to retain all modified sequences, flagging them with a p…

75124a9

…lus in case its a decoy

Revert "added option to retain all modified sequences, flagging them …

07dd041

…with a plus in case its a decoy" This reverts commit 75124a9.

added keepall functionality

f2046bd

added CLI keep_all explanation

db77736

Revert "Proper type hinting and some more docstrings"

24038d6

This reverts commit 85978da.

Added type conversion to prevent FutureWarning form Pandas because a …

8c46708

…string of an integer is added to a float series

Removed .to_numpy() to prevent errors with concatenated values

2370577

Added string conversion to allow usage of .str accessor in line 120

ec3a135

Removed .to_numpy() again...

13bcf3b

Fixed indexing issue due to missing .loc[]

b88852e

List conversion to allow explode step

0feb1a5

Fixed UnboundLocalError raised when skip_evidence is True and we try …

b4c6a19

…to delete a table that does not exist

Changed column merge handling

0458114

Changed from pipe-separated strings and splitting to just using lists…

7002c78

… from start

bump pyarrow version for Oktoberfest

4df299a

fhamood requested a review from Copilot May 20, 2025 14:47

Copilot AI reviewed May 20, 2025

View reviewed changes

fhamood and others added 3 commits May 20, 2025 16:53

Merge branch 'master' into keep_all_modseq

770c30c

Update simsi_transfer/main.py

b88a75b

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Ready for merge

a2776e4

fhamood merged commit e1387ac into master May 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Keep all modseq mode for ProSIMSIt #31

Keep all modseq mode for ProSIMSIt #31

Uh oh!

fhamood commented May 20, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI May 20, 2025

Uh oh!

Copilot AI May 20, 2025

Uh oh!

Copilot AI May 20, 2025

Uh oh!

Uh oh!

Copilot AI May 20, 2025

Uh oh!

Copilot AI May 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		return ";".join(sorted(set([x for x in s.split(";") if len(x) > 0])))


		def list_all(x: pd.Series):

-def list_all(x: pd.Series):
+def list_all(x: pd.Series) -> List[Any]:
+    """
+    Converts a pandas Series to a list.
+    :param x: A pandas Series object.
+    :return: A list containing the elements of the Series.
+    """



		def transfer(summary_df, max_pep=False, mask=False, ambiguity_decision='majority'):
		def transfer(summary_df, max_pep=False, mask=False, ambiguity_decision='majority', overwrite=False):

-    :param tempfile: Temporary processing file, e.g., MaRaCluster clustering dataframe
-    :param msms_df: MaxQuant msms.txt dataframe
-    :return: Merged dataframe
+    :param summary: Summary dataframe containing information about MS2 scans.
+    :param evidence: Evidence dataframe containing precursor information.
+    :param allpeptides: AllPeptides dataframe containing peptide-level information.
+    :param plex: Integer specifying the plex level for the experiment.
+    :param num_threads: Number of threads to use for parallel processing.
+    :return: A concatenated dataframe containing merged evidence information.

Keep all modseq mode for ProSIMSIt #31

Keep all modseq mode for ProSIMSIt #31

Uh oh!

Conversation

fhamood commented May 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants