Peptide Sequence Plotting. Solves issue #53 #67
Conversation
WalkthroughThe changes introduce peptide sequence plotting functionality into the project. A new method, Changes
Sequence Diagram(s)sequenceDiagram
participant S as SpectrumPlot
participant C as SpectrumConfig
participant M as MATPLOTLIBSpectrumPlot
participant U as UnsupportedBackend
S->>C: Check if display_peptide_sequence is enabled
alt Enabled (Matplotlib)
S->>M: Call plot_peptide_sequence(peptide_sequence, matched_fragments)
M->>M: Render text and annotations via ax.text/annotate
else Unsupported
S->>U: Call plot_peptide_sequence
U-->>S: Raise NotImplementedError
end
Possibly related issues
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pyopenms_viz/_matplotlib/core.py (1)
586-619: Well-structured implementation of peptide sequence visualization.Good implementation that:
- Places the peptide sequence in the upper left corner with configurable styling
- Conditionally handles matched fragments with arrow annotations
- Uses configuration properties for consistent visual styling
Consider adding a test case to verify this functionality works as expected with different peptide sequences and matched fragment combinations.
pyopenms_viz/_config.py (1)
365-372: Well-structured implementation of peptide sequence plotting configuration.The new fields added to the
SpectrumConfigclass provide a comprehensive set of customization options for peptide sequence plotting. The default values are sensible and follow the established patterns in the codebase.Consider adding more specific type annotations for
matched_fragments. Whilelist[tuple]works, a more precise type likelist[tuple[float, float]](if these tuples represent m/z and intensity values) would improve type safety and code clarity.- matched_fragments: list[tuple] = field(default_factory=list) + matched_fragments: list[tuple[float, float]] = field(default_factory=list)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pyopenms_viz/_bokeh/core.py(1 hunks)pyopenms_viz/_config.py(1 hunks)pyopenms_viz/_core.py(2 hunks)pyopenms_viz/_matplotlib/core.py(1 hunks)pyopenms_viz/_plotly/core.py(1 hunks)test/test_chromatogram.py(2 hunks)
🔇 Additional comments (6)
pyopenms_viz/_core.py (2)
692-698: Good abstract method declaration.The abstract method
plot_peptide_sequencefollows best practices with clear parameter types, descriptive docstring, and consistent naming.
807-813: Well-implemented conditional peptide sequence plotting.The conditional logic checks both required config parameters before invoking the method, providing a clean integration with the existing plotting flow.
pyopenms_viz/_plotly/core.py (1)
623-627: Correct implementation of unsupported feature.The method properly raises a NotImplementedError with a clear message explaining that peptide sequence plotting isn't supported in the Plotly backend.
pyopenms_viz/_bokeh/core.py (1)
548-552: Consistent error handling for unsupported feature.The NotImplementedError implementation matches the approach used in the Plotly backend, providing a clear message about the lack of support for peptide sequence plotting.
test/test_chromatogram.py (2)
8-11: Good practice using non-interactive Matplotlib backend.Setting the backend to "Agg" prevents GUI-related issues during testing, which is especially important for CI environments where a display server might not be available.
84-96: Comprehensive testing for unsupported backends.This parametrized test properly verifies that attempting to use peptide sequence plotting with unsupported backends raises the appropriate exception. The error message matching is also a good practice.
One small inconsistency: This test also uses
kind="spectrum"despite being in thetest_chromatogram.pyfile, similar to the issue in the previous test.
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
Coming along, thank you for addressing my comments. Althuogh the tests pass there still seems to be some issues. Mainly, it seems that the tests that you added all seem to be skipped. Possibly you forgot to add the snapshots for the new tests? These can be generated with --snapshot-update Also I am unsure why some of the snapshots were updated. Can you please double check (e.g. post screenshots of new and old snapshots) to ensure there are no major changes.
pyopenms_viz/_core.py
Outdated
| if self._config.display_peptide_sequence and self._config.peptide_sequence: | ||
| self.plot_peptide_sequence( | ||
| self._config.peptide_sequence, | ||
| matched_fragments=self._config.matched_fragments |
There was a problem hiding this comment.
Should be able to reference directly (e.g. self.display_peptide_sequence, self.peptide_sequence instead of self._config.display_peptide_sequence)
test/__snapshots__/test_peakmap_marginals/test_peakmap_marginals[ms_bokeh].html
Outdated
Show resolved
Hide resolved
test/__snapshots__/test_peakmap_marginals/test_peakmap_mz_im[ms_bokeh].html
Outdated
Show resolved
Hide resolved
test/test_chromatogram.py
Outdated
| def test_spectrum_peptide_sequence_matplotlib(chromatogram_data, snapshot): | ||
| kwargs = { | ||
| "display_peptide_sequence": True, | ||
| "peptide_sequence": "PEPTIDEABC", |
There was a problem hiding this comment.
nitpicky comment please change "PEPTIDEABC" to "PEPTIDEK" as this is a more standard dummy example in the proteomics community
test/test_chromatogram.py
Outdated
| ) | ||
| fig = out.get_figure() | ||
| fig.tight_layout() | ||
| assert snapshot == out |
There was a problem hiding this comment.
please add this snapshot
test/test_chromatogram.py
Outdated
| kind="spectrum", | ||
| show_plot=False, | ||
| display_peptide_sequence=True, | ||
| peptide_sequence="PEPTIDEXYZ", |
There was a problem hiding this comment.
See above, change to "PEPTIDEK"
test/test_chromatogram.py
Outdated
| out = chromatogram_data.plot( | ||
| x="rt", | ||
| y="int", | ||
| kind="spectrum", |
There was a problem hiding this comment.
tests should be added to test_spectrum instead of test_chromatogram
There was a problem hiding this comment.
Please see minor comments below regarding _config. I also edited the tests so now they are not skipped.
While testing on my local machine, I noticed that none of the fragmentation is currently tested/displayed and I am not sure if you implemented this functionality. Please double check #29 what we want is that in the corner of the spectrum we show the peptide sequence as well as its fragmentation pattern (all the lines, in different colors). Note that the ion_annotation and ion_color are specified in the dataframe so we would not need to pass extra arguments.
Please use test/testSpectrumDf.tsv to help with your testing. The sequence is DMAGCH (same as #29 ) and we are expecting y9 to be off just like in that example.
Pleae post an image of the spectrum with this functionality in this PR.
I tried to update the changes to this PR however I do not have the permissions to do so. Please update the tests as per this branch https://github.com/OpenMS/pyopenms_viz/tree/sans/main to ensure that the matplotlib test is not skipped as well.
test/test_chromatogram.py
Outdated
| @pytest.mark.parametrize("backend", ["ms_bokeh", "ms_plotly"]) | ||
| def test_chromatogram_peptide_sequence_unsupported(chromatogram_data, backend): | ||
| pd.options.plotting.backend = backend | ||
| with pytest.raises(NotImplementedError, match="unsupported"): | ||
| chromatogram_data.plot( | ||
| x="rt", | ||
| y="int", | ||
| kind="spectrum", | ||
| show_plot=False, | ||
| display_peptide_sequence=True, | ||
| peptide_sequence="PEPTIDEK", | ||
| ) |
There was a problem hiding this comment.
this test should be in test_spectrum
pyopenms_viz/_matplotlib/core.py
Outdated
| arrowprops=dict(arrowstyle="->", color=self._config.highlight_color), | ||
| fontsize=self._config.peptide_sequence_fontsize, | ||
| color=self._config.highlight_color, | ||
| alpha=self._config.highlight_alpha, |
There was a problem hiding this comment.
should be able to use self.highlight_color directly etc
pyopenms_viz/_matplotlib/core.py
Outdated
| fontsize=self._config.peptide_sequence_fontsize, | ||
| color=self._config.peptide_sequence_color, |
There was a problem hiding this comment.
should be able to use self.peptide_sequence_fontsize directly (remove _config)
pyopenms_viz/_config.py
Outdated
| peptide_sequence_fontsize: int = 12 | ||
| peptide_sequence_color: str = "red" | ||
| highlight_color: str = "blue" | ||
| highlight_alpha: float = 0.8 |
There was a problem hiding this comment.
Please see comments below, some of these arguments might not be needed because they are expected to be specified in the dataframe. (lines 365- 372)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pyopenms_viz/_matplotlib/core.py (4)
597-603: Clear docstring with appropriate explanation.The docstring clearly explains the purpose of the method and how it works. It would be helpful to add parameter descriptions for completeness.
Consider expanding the docstring to include parameter descriptions:
- """ - Plot peptide sequence with matched fragments indicated. - - The peptide is displayed in the top corner and the fragmentation pattern - is drawn as lines coming out from the letters -- using the dataframe columns - 'ion_annotation' and 'ion_color' (or 'color_annotation'). No extra arguments are needed. - """ + """ + Plot peptide sequence with matched fragments indicated. + + The peptide is displayed in the top corner and the fragmentation pattern + is drawn as lines coming out from the letters -- using the dataframe columns + 'ion_annotation' and 'ion_color' (or 'color_annotation'). No extra arguments are needed. + + Parameters + ---------- + peptide_sequence : str + The peptide sequence to display + matched_fragments : list, optional + List of matched fragments as (m/z, intensity) pairs + x : float, default=0.5 + Center horizontal position (axes fraction) + y : float, default=0.95 + Vertical position (axes fraction) + spacing : float, default=0.05 + Spacing between residues (axes fraction) + fontsize : int, default=12 + Font size for peptide sequence + fontsize_frag : int, default=10 + Font size for fragment annotations + frag_len : float, default=0.05 + Length for fragment lines (in axes fraction) + """
623-650: Well-handled fragment extraction logic.The fragment extraction logic is robust, with appropriate fallbacks and error handling. It correctly handles different ion types and positioning.
Consider refactoring the ion type detection into a helper method to improve maintainability.
- if ion_type in "abc": - y_offset = frag_len - elif ion_type in "xyz": - y_offset = -frag_len - else: - y_offset = 0 + y_offset = self._get_fragment_y_offset(ion_type, frag_len)And add a helper method:
def _get_fragment_y_offset(self, ion_type, frag_len): """ Determine the y-offset for a fragment based on its ion type. Parameters ---------- ion_type : str The type of ion (a, b, c, x, y, z) frag_len : float The length of the fragment line Returns ------- float The y-offset for the fragment """ if ion_type in "abc": return frag_len elif ion_type in "xyz": return -frag_len else: return 0
651-681: Good fragment rendering implementation.The fragment rendering code handles different fragment formats well and includes appropriate fallbacks for non-string colors. The visualization logic with lines and text is implemented properly.
Based on previous review comments, you might want to use class attributes if they're available:
- color=frag_color, + color=frag_color if not hasattr(self, "highlight_color") else self.highlight_color,And:
- fontsize=fontsize_frag, + fontsize=fontsize_frag if not hasattr(self, "peptide_sequence_fontsize") else self.peptide_sequence_fontsize,
586-681: Consider adding type annotations for improved code readability.The method would benefit from complete type annotations, especially for the
matched_fragmentsparameter.- def plot_peptide_sequence( - self, - peptide_sequence: str, - matched_fragments=None, - x: float = 0.5, # center horizontal position (axes fraction) - y: float = 0.95, # vertical position (axes fraction) - spacing: float = 0.05, # spacing between residues (axes fraction) - fontsize: int = 12, - fontsize_frag: int = 10, - frag_len: float = 0.05 # length for fragment lines (in axes fraction) - ): + def plot_peptide_sequence( + self, + peptide_sequence: str, + matched_fragments: list[tuple] | None = None, + x: float = 0.5, # center horizontal position (axes fraction) + y: float = 0.95, # vertical position (axes fraction) + spacing: float = 0.05, # spacing between residues (axes fraction) + fontsize: int = 12, + fontsize_frag: int = 10, + frag_len: float = 0.05 # length for fragment lines (in axes fraction) + ) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/__snapshots__/test_spectrum/test_spectrum_peptide_sequence_matplotlib[ms_matplotlib].pngis excluded by!**/*.png
📒 Files selected for processing (4)
pyopenms_viz/_config.py(1 hunks)pyopenms_viz/_matplotlib/core.py(1 hunks)test/test_chromatogram.py(2 hunks)test/test_spectrum.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test_chromatogram.py
- pyopenms_viz/_config.py
🧰 Additional context used
🧬 Code Definitions (2)
test/test_spectrum.py (3)
test/conftest.py (2) (2)
spectrum_data(76-77)snapshot(41-50)pyopenms_viz/_matplotlib/core.py (4) (4)
plot(294-316)plot(325-385)plot(405-469)plot(701-718)pyopenms_viz/_matplotlib/__init__.py (1) (1)
plot(33-41)
pyopenms_viz/_matplotlib/core.py (1)
pyopenms_viz/_core.py (6) (6)
plot_peptide_sequence(693-698)plot(343-347)plot(580-602)plot(647-649)plot(757-862)plot(1185-1197)
🔇 Additional comments (5)
test/test_spectrum.py (3)
9-11: Good setup for matplotlib in tests.The explicit setting of the Matplotlib backend to "Agg" ensures tests will run in headless environments, which is perfect for CI/CD pipelines.
99-118: Well-structured test for peptide sequence visualization.This test appropriately verifies that the Matplotlib backend correctly handles peptide sequence plotting with matched fragments. The test provides good coverage of the new functionality.
A few observations:
- The docstring clearly explains the test's purpose
- The test uses appropriate test data with dummy peptide sequence and matched fragments
- The snapshot testing approach ensures visual consistency
120-131: Good test for unsupported backends.This test correctly verifies that attempting to use peptide sequence plotting with unsupported backends raises the appropriate exception. The parametrization ensures both Bokeh and Plotly backends are tested.
pyopenms_viz/_matplotlib/core.py (2)
586-596: Well-defined method signature with good defaults.The method signature is clear with descriptive parameter names and appropriate default values. The comments on each parameter help readers understand what each parameter does without needing to read the docstring.
606-621: Good centering logic and amino acid rendering.The centering calculation is elegant and ensures the peptide sequence is properly positioned. The rendering of individual amino acids is implemented correctly.
2 participants
Hey @jcharkow !
This PR addresses issue #53 by integrating the feature to plot peptide sequences with matched fragments into the Matplotlib backend interface.
I have followed your instructions from the issue description.
Also followed your instructions on the pr #62 .
Summary by CodeRabbit
New Features
Tests