Skip to content

Handle mixed CDR3 lengths in logoplots#692

Draft
AtharvaKatiyar wants to merge 1 commit into
scverse:mainfrom
AtharvaKatiyar:fix-logoplot-mixed-lengths
Draft

Handle mixed CDR3 lengths in logoplots#692
AtharvaKatiyar wants to merge 1 commit into
scverse:mainfrom
AtharvaKatiyar:fix-logoplot-mixed-lengths

Conversation

@AtharvaKatiyar
Copy link
Copy Markdown

Summary

Fixes #691.

pl.logoplot_cdr3_motif now handles mixed CDR3 sequence lengths by automatically creating one logo subplot per detected length.

Changes

  • Group CDR3 sequences by length before calling alignment_to_matrix.
  • Keep existing behavior for equal-length input (returns a single logomaker.Logo).
  • For mixed lengths, auto-create stacked subplots and return list[logomaker.Logo].
  • Raise clear ValueError when mixed lengths are provided with a single user-supplied ax.

Validation

  • pytest -q src/scirpy/tests/test_plotting.py -k logoplot
  • pytest -q (983 passed, 2 deselected)

Automatically split CDR3 sequences by length in pl.logoplot_cdr3_motif and generate one logo subplot per length. Preserve single-logo behavior for equal-length inputs, and raise a clear error when mixed lengths are provided with a single pre-defined axis.

Add regression tests covering mixed-length return behavior and single-axis error handling.

Validated with full test suite: 983 passed, 2 deselected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 29, 2026 20:13
Copy link
Copy Markdown

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 fixes scirpy issue #691 by enabling pl.logoplot_cdr3_motif to handle mixed CDR3 sequence lengths: it now splits sequences by length and renders one logo plot per detected length instead of failing.

Changes:

  • Group CDR3 sequences by length and generate one logo plot per length when mixed lengths are present.
  • Preserve existing single-length behavior (returning a single logomaker.Logo) and return list[logomaker.Logo] only for mixed lengths.
  • Add tests for mixed-length behavior and for the error raised when a single user-supplied ax is provided with mixed lengths.

Reviewed changes

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

File Description
src/scirpy/pl/_logoplots.py Implements length-splitting logic, multi-subplot rendering, updated return type, and a clearer error for ax misuse.
src/scirpy/tests/test_plotting.py Adds regression tests covering mixed-length return type and the single-ax error path.

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

sequences_by_length: dict[int, list[str]] = {}
for sequence in sequence_list:
sequences_by_length.setdefault(len(sequence), []).append(sequence)
n_lengths = len(sequences_by_length)
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

sequence_list can be empty (e.g., when the selected chains/cdr3_col are all missing after subsetting). In that case n_lengths becomes 0 and the function falls through to plt.subplots(n_lengths, ...), which will raise a low-level Matplotlib error (nrows must be > 0). Add an explicit check for no sequences and raise a clear ValueError (e.g., "No CDR3 sequences found for the selected chains/column").

Suggested change
n_lengths = len(sequences_by_length)
n_lengths = len(sequences_by_length)
if n_lengths == 0:
raise ValueError("No CDR3 sequences found for the selected chains/column")

Copilot uses AI. Check for mistakes.
Comment on lines 78 to 79
Parameters passed to the :func:`matplotlib.pyplot.figure` call
if no `ax` is specified.
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

The docstring says fig_kws is passed to matplotlib.pyplot.figure, but this function uses _init_ax(fig_kws) / plt.subplots(..., **fig_kws) (and _init_ax itself wraps plt.subplots). Please update the parameter description to match the actual call site (i.e. plt.subplots/figure creation kwargs) so users know which keys are supported.

Suggested change
Parameters passed to the :func:`matplotlib.pyplot.figure` call
if no `ax` is specified.
Parameters passed to the axes/figure creation call via
:func:`matplotlib.pyplot.subplots` if no `ax` is specified.

Copilot uses AI. Check for mistakes.
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

❌ Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.18%. Comparing base (2ce57d1) to head (0b12a3a).

Files with missing lines Patch % Lines
src/scirpy/pl/_logoplots.py 0.00% 32 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
- Coverage   19.30%   19.18%   -0.13%     
==========================================
  Files          51       51              
  Lines        4636     4655      +19     
==========================================
- Hits          895      893       -2     
- Misses       3741     3762      +21     
Files with missing lines Coverage Δ
src/scirpy/pl/_logoplots.py 21.15% <0.00%> (-12.18%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@grst grst marked this pull request as draft May 11, 2026 18: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.

Handle sequences with different length in logoplots

2 participants