Skip to content

fix: added decoy col to confidence reports#137

Open
jspaezp wants to merge 3 commits into
wfondrie:mainfrom
jspaezp:feat/re_add_confidence_api
Open

fix: added decoy col to confidence reports#137
jspaezp wants to merge 3 commits into
wfondrie:mainfrom
jspaezp:feat/re_add_confidence_api

Conversation

@jspaezp

@jspaezp jspaezp commented Mar 12, 2025

Copy link
Copy Markdown
Collaborator

Purpose

Re-adds the decoy column in the reports

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Has linting/formatting been run? (make lint, make format, make check)
  • Was it tested locally? (make test)
  • Has the changelog been updated?
  • Have the docs been updated? (I might need help with this one ... we need a smart way to get percolator available)
  • The level of testing this PR includes is appropriate.
  • No features have been dropped (or note which ones and why).

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, Optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

Notes

(FILL ME IN, Optional) Additional context to this PR.

@jspaezp
jspaezp marked this pull request as ready for review March 19, 2025 06:29
@jspaezp jspaezp changed the title [WIP] fix: added decoy col to confidence reports fix: added decoy col to confidence reports Apr 10, 2025
@wfondrie
wfondrie requested a review from Copilot February 5, 2026 17:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request re-adds the "decoy" column to confidence reports and drops support for Python 3.9. The main purpose is to ensure that the is_decoy column is consistently present in all output formats, fixing an issue where it was disappearing in certain output types.

Changes:

  • Added is_decoy to the standard column name mapping for consistent column naming across the codebase
  • Refactored file writer initialization logic to prevent overwriting existing files and handle multi-dataset aggregation scenarios
  • Dropped Python 3.9 support, updated minimum required version to Python 3.10

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pyproject.toml Updated minimum Python version from 3.9 to 3.10
uv.lock Removed Python 3.9-specific package versions, updated resolution markers
mokapot/column_defs.py Added "is_decoy" to STANDARD_COLUMN_NAME_MAP
mokapot/confidence.py Updated to use standardized "is_decoy" column name, improved writer initialization tracking
mokapot/rollup.py Refactored get_target_decoy_reader to validate is_decoy column presence
mokapot/tabular_data/csv.py Added overwrite parameter and file existence validation with better error messages
mokapot/tabular_data/streaming.py Added duplicate column name detection in data iteration
mokapot/parsers/pin.py Added type annotation for max_workers parameter
tests/unit_tests/test_confidence.py Added test_confidence_api to verify decoy column inclusion
tests/unit_tests/test_writer_flashlfq.py Added dest_dir parameter to fix test cleanup
tests/unit_tests/test_tabular_data.py Added overwrite=True to MockWriter initialization
tests/system_tests/test_cli.py Parameterized test_cli_aggregate, fixed test_cli_saved_models directory conflicts
tests/system_tests/test_brew_rollup.py Changed fixture scope to prevent directory conflicts
Makefile Added vignette execution and cleanup commands

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mokapot/rollup.py
)
return reader

raise RuntimeError("Column 'is_decoy' not found in reader.")

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

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

The code always raises a RuntimeError if the "is_decoy" column is not found in the reader, even though the function is supposed to handle this case by validating the existing column. This will break for any input files that don't have the "is_decoy" column.

The commented-out code below suggests there was previously a fallback to compute the column if it didn't exist. If files without "is_decoy" are expected, this unconditional raise will cause failures. Consider either:

  1. Removing the raise and uncommenting the fallback code if files without "is_decoy" are valid
  2. Documenting that "is_decoy" column is now required in input files

Copilot uses AI. Check for mistakes.
Comment thread mokapot/rollup.py
Comment on lines +56 to +65
# Pretty sure this is dead code now ...
# Former implementations did no store the decoy state
# on the reader ... this made the column dissapear in
# some output types.
# return ComputedTabularDataReader(
# reader=reader,
# column="is_decoy",
# dtype=np.dtype("bool"),
# func=lambda df: np.full(len(df), is_decoy),
# )

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

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

Dead code comment indicates uncertainty about whether this code should be removed. Since the raise on line 55 makes it unreachable, either:

  1. Remove this commented code if it's truly obsolete
  2. Remove the unconditional raise on line 55 and uncomment this code if it's still needed as a fallback

The comment "Pretty sure this is dead code now" suggests the developer is uncertain, which indicates this needs clarification before merging.

Suggested change
# Pretty sure this is dead code now ...
# Former implementations did no store the decoy state
# on the reader ... this made the column dissapear in
# some output types.
# return ComputedTabularDataReader(
# reader=reader,
# column="is_decoy",
# dtype=np.dtype("bool"),
# func=lambda df: np.full(len(df), is_decoy),
# )

Copilot uses AI. Check for mistakes.
Comment thread mokapot/rollup.py
raise RuntimeError("Column 'is_decoy' not found in reader.")
# Pretty sure this is dead code now ...
# Former implementations did no store the decoy state
# on the reader ... this made the column dissapear in

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

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

Typo in comment: "dissapear" should be "disappear"

Suggested change
# on the reader ... this made the column dissapear in
# on the reader ... this made the column disappear in

Copilot uses AI. Check for mistakes.
raise FileExistsError(msg)

df = pd.DataFrame(columns=self.columns)
df.to_csv(self.file_name, **self.stdargs)

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

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

The initialized flag is set to False in init but is never set to True in the initialize() method. This means that after the first call to initialize(), subsequent calls will still show initialized=False and won't trigger the early return warning on line 110. The flag should be set to True at the end of the initialize() method after successful initialization.

Suggested change
df.to_csv(self.file_name, **self.stdargs)
df.to_csv(self.file_name, **self.stdargs)
self.initialized = True

Copilot uses AI. Check for mistakes.
Comment thread mokapot/confidence.py
@@ -1092,11 +1115,15 @@ def build_writers(
str(level_manager.default_extension),
]
outfile_decoys = level_manager.dest_dir / "".join(decoy_name)

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

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

The initialization logic for decoy writers doesn't reset should_init before checking the condition. On line 1097, should_init is set based on append_to_output_file, then on line 1098-1100 it may be set to False if the targets file was already initialized. However, when processing the decoys file (line 1118-1120), should_init still retains the value from the targets file processing.

This means if the targets file was already initialized, should_init will be False for decoys even if the decoys file hasn't been initialized yet. The code should reset should_init to not self.append_to_output_file before line 1118, similar to line 1097.

Suggested change
outfile_decoys = level_manager.dest_dir / "".join(decoy_name)
outfile_decoys = level_manager.dest_dir / "".join(decoy_name)
should_init = not self.append_to_output_file

Copilot uses AI. Check for mistakes.
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