Skip to content

fix(msstats+): Fix lookup of protein IDs for profile plots#136

Merged
tonywu1999 merged 4 commits intodevelfrom
BugFix-SumarizationPlot-not-showoing-protein
Dec 2, 2025
Merged

fix(msstats+): Fix lookup of protein IDs for profile plots#136
tonywu1999 merged 4 commits intodevelfrom
BugFix-SumarizationPlot-not-showoing-protein

Conversation

@Rudhik1904
Copy link
Copy Markdown
Contributor

@Rudhik1904 Rudhik1904 commented Nov 29, 2025

Fix: Replace index-based column lookup with name-based lookup

Processing the run order file reorders the in-memory data, shifting the 'ProteinName' column. The dropdown logic relied on a fixed index, leading to data mismatches.

Updated the code to target the column by name ('ProteinName') to ensure stability regardless of column order.

Summary by CodeRabbit

  • Bug Fixes

    • Protein selection dropdown in QC analysis now uses the designated protein-name field, ensuring correct and consistent protein choices across modes.
  • Documentation

    • Examples updated to show the default summary method set to "TMP" in QC/config documentation.
  • Tests

    • Test examples and expectations updated to reflect the "TMP" summary method.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Nov 29, 2025

Walkthrough

Replaced protein-choice source in QC server UI to use ProteinName for non-PTM flows; added qc_input$summaryMethod = "TMP" to example snippets and Rd docs; updated tests to use "TMP" as the summaryMethod.

Changes

Cohort / File(s) Summary
Protein selection data source
R/module-qc-server.R
Replaced unique(get_data()[1]) with unique(get_data()$ProteinName) in two non-PTM branches that populate protein choices.
Examples & documentation
R/main_calculations.R, man/lf_summarization_loop.Rd, man/tmt_summarization_loop.Rd, man/tmt_model.Rd
Added qc_input$summaryMethod = "TMP" to example usage snippets in roxygen blocks and Rd examples.
Tests updated
tests/testthat/test-utils.R
Updated test inputs and expectations to use summaryMethod = "TMP" instead of "linear" in several test cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to:
    • R/module-qc-server.R — ensure ProteinName exists for all expected input shapes and that PTM-related branches are unaffected.
    • tests/testthat/test-utils.R — confirm updated expectations match actual summarization behavior when using "TMP".
    • Example/docs (R/main_calculations.R, man/*.Rd, man/tmt_model.Rd) — verify examples stay consistent with implementation and do not mislead users.

Possibly related PRs

Suggested labels

Review effort 3/5

Poem

🐇 I hopped through code and docs at dusk,
Swapped names and nudged examples to "TMP" — no fuss.
ProteinName now leads the chase,
Tests and examples fall in place. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title directly addresses the main fix: changing from index-based to name-based lookup of protein IDs for profile plots, which aligns with the primary change across R/module-qc-server.R and documentation updates.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch BugFix-SumarizationPlot-not-showoing-protein

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/testthat/test-utils.R (1)

995-995: Tests now correctly drive the TMP summarization path in QC flows

Setting mock_input$summaryMethod = "TMP" in these preprocessData QC tests matches the updated summarization behavior and the documented example; the stubbing pattern remains sound.

If you want stronger regression protection later, you could additionally assert that the returned QC object’s SummaryMethod (where present) is actually "TMP", not just that the name exists.

Also applies to: 1026-1026, 1087-1087

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 55d6459 and 233604d.

📒 Files selected for processing (3)
  • R/main_calculations.R (1 hunks)
  • man/lf_summarization_loop.Rd (1 hunks)
  • tests/testthat/test-utils.R (5 hunks)
✅ Files skipped from review due to trivial changes (1)
  • R/main_calculations.R
🔇 Additional comments (2)
man/lf_summarization_loop.Rd (1)

50-50: Doc example correctly exposes summaryMethod usage

Adding qc_input$summaryMethod = "TMP" to the example keeps docs in sync with the QC pipeline and tests; no issues spotted.

tests/testthat/test-utils.R (1)

1221-1221: dataComparison tests consistently aligned with TMP summary method

Using mock_input$summaryMethod = "TMP" in these dataComparison tests keeps the modeling flows consistent with the updated label-free summarization method; no functional or mocking issues detected.

Also applies to: 1325-1325

@tonywu1999 tonywu1999 changed the title Instead of using the index to get the right column, I am using the co… fix(msstats+): Fix lookup of protein IDs for profile plots Dec 2, 2025
@tonywu1999 tonywu1999 merged commit 4ade836 into devel Dec 2, 2025
2 checks passed
@tonywu1999 tonywu1999 deleted the BugFix-SumarizationPlot-not-showoing-protein branch December 2, 2025 01:54
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