Skip to content

Conversation

@matteopilz
Copy link
Contributor

@matteopilz matteopilz commented Sep 1, 2025

Summary by CodeRabbit

  • New Features

    • Introduced PeptideIdentificationList as a standard container for managing peptide identifications in the public API.
  • Documentation

    • Updated user guides across multiple topics to reflect the new peptide identification container usage patterns.
  • Chores

    • Added jupyter_bokeh dependency to requirements.

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

@coderabbitai
Copy link

coderabbitai bot commented Sep 1, 2025

Walkthrough

This PR updates documentation examples across multiple user guide files to replace Python list initialization ([]) with the new oms.PeptideIdentificationList() container for holding peptide identifications. Additionally, corresponding API calls are updated from .append() and .len() to .push_back() and .size() respectively. A jupyter_bokeh dependency is added to requirements.txt.

Changes

Cohort / File(s) Summary
Documentation: PeptideIdentificationList container adoption
docs/source/user_guide/PSM_to_features.rst, docs/source/user_guide/export_pandas_dataframe.rst, docs/source/user_guide/other_ms_data_formats.rst, docs/source/user_guide/quality_control.rst
Replace peptide_ids = [] or pep_ids = [] with oms.PeptideIdentificationList() in example code blocks.
Documentation: PeptideIdentificationList with method updates
docs/source/user_guide/identification_data.rst, docs/source/user_guide/untargeted_metabolomics_preprocessing.rst
Replace list initialization with oms.PeptideIdentificationList() and update method calls from .append() to .push_back().
Documentation: Size check updates
docs/source/user_guide/export_files_GNPS.rst
Replace truthiness check if feature.getPeptideIdentifications(): with explicit size check if feature.getPeptideIdentifications().size() > 0:.
Documentation: Length method updates
docs/source/user_guide/peptide_search.rst
Replace peptide_ids = [] with oms.PeptideIdentificationList() and update result counting from len(peptide_ids) to peptide_ids.size().
Dependencies
requirements.txt
Add jupyter_bokeh dependency after bokeh.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

These are straightforward, repetitive pattern replacements across documentation examples. The changes consistently replace Python lists with the new container type and update corresponding method calls in a uniform manner. The requirements.txt change is trivial. No complex logic or architectural decisions are involved.

Possibly related issues

Poem

🐰 A list becomes a box so fine,
With push_back() in place of line,
PyOpenMS shows a better way,
Containers dance where lists once lay! 📦

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.
Title check ✅ Passed The title accurately summarizes the main change across all files: updating documentation examples to use oms.PeptideIdentificationList instead of Python lists for peptide identification containers.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 132f3f7 and c720b85.

📒 Files selected for processing (1)
  • requirements.txt (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-test
  • GitHub Check: build-test
🔇 Additional comments (1)
requirements.txt (1)

9-9: Confirm jupyter_bokeh necessity and version compatibility

Adding jupyter_bokeh alongside bokeh makes sense if the docs/notebooks actually rely on the Jupyter extension, but it does add another runtime dependency. Please double‑check that:

  • The examples (or the docs build/binder image) really require jupyter_bokeh, and
  • Its version is compatible with the Bokeh and Jupyter stack used for rendering the docs.

If both hold, this addition looks good to ship.


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.

Copy link

@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 (7)
requirements.txt (1)

9-9: Add version pin or constraints for jupyter_bokeh (optional).

Unpinned deps can drift and break the examples, especially with Bokeh/HoloViews stacks. Either pin jupyter_bokeh or add a constraints/lock file for the docs env to keep interactive plots stable.

docs/source/user_guide/interactive_plots.rst (1)

80-85: Nit: prefer standard axis label “m/z”.

Minor terminology polish for consistency with MS literature.

Apply this diff:

-hd.dynspread(raster, threshold=0.7, how="add", shape="square").opts(
-    width=800,
-    height=800,
-    xlabel="Retention time (s)",
-    ylabel="mass/charge (Da)",
-)
+hd.dynspread(raster, threshold=0.7, how="add", shape="square").opts(
+    width=800,
+    height=800,
+    xlabel="Retention time (s)",
+    ylabel="m/z",
+)
docs/source/user_guide/other_ms_data_formats.rst (2)

20-22: Align prose with new container type.

Since examples now use oms.PeptideIdentificationList, consider clarifying in the surrounding text that it’s a list-like container of PeptideIdentification.


51-53: Repeat the container note for pepXML.

Same suggestion as above to avoid reader confusion about accepted types.

docs/source/user_guide/export_pandas_dataframe.rst (1)

113-117: Update parameter doc to accept Iterable[PeptideIdentification].

peptide_identifications_to_df currently documents “list”; since the example passes PeptideIdentificationList, note that any iterable/list-like container is supported (if true). Otherwise, convert to list before calling.

docs/source/user_guide/peptide_search.rst (1)

146-149: Use of size() is consistent with the new container.

Good update from len(peptide_ids) to peptide_ids.size(). Ensure other docs follow the same convention.

docs/source/user_guide/quality_control.rst (1)

45-49: Update the inline comment to match the new container.

The code now uses a container, not a Python list.

-    pep_ids = oms.PeptideIdentificationList()  # list of PeptideIdentification()
+    pep_ids = oms.PeptideIdentificationList()  # PeptideIdentificationList container
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2332706 and 132f3f7.

📒 Files selected for processing (10)
  • docs/source/user_guide/PSM_to_features.rst (1 hunks)
  • docs/source/user_guide/export_files_GNPS.rst (1 hunks)
  • docs/source/user_guide/export_pandas_dataframe.rst (1 hunks)
  • docs/source/user_guide/identification_data.rst (2 hunks)
  • docs/source/user_guide/interactive_plots.rst (2 hunks)
  • docs/source/user_guide/other_ms_data_formats.rst (3 hunks)
  • docs/source/user_guide/peptide_search.rst (4 hunks)
  • docs/source/user_guide/quality_control.rst (1 hunks)
  • docs/source/user_guide/untargeted_metabolomics_preprocessing.rst (2 hunks)
  • requirements.txt (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-test
  • GitHub Check: build-test
🔇 Additional comments (9)
docs/source/user_guide/export_files_GNPS.rst (1)

50-51: Good: explicit size() check improves clarity with the new container.

This avoids relying on truthiness and reads well with PeptideIdentificationList semantics.

docs/source/user_guide/interactive_plots.rst (1)

38-39: Document the 5th argument to get2DPeakDataLong.

Please add a short note explaining what the “1” controls (e.g., MS level, binning, or threads) so readers know how to adjust it.

docs/source/user_guide/other_ms_data_formats.rst (1)

34-36: Consistency check: container works with load/store.

Assuming IdXML/mzIdentML bindings accept PeptideIdentificationList seamlessly. If there are versions that still expect Python lists, add a brief note on the minimum pyOpenMS version here.

docs/source/user_guide/peptide_search.rst (1)

34-37: Switch to PeptideIdentificationList looks correct.

Using oms.PeptideIdentificationList for SimpleSearchEngineAlgorithm.search is appropriate and iteration below remains compatible.

docs/source/user_guide/PSM_to_features.rst (1)

48-51: Loading into PeptideIdentificationList is correct.

IdXMLFile.load accepts the new container; downstream IDMapper.annotate call matches this type.

docs/source/user_guide/untargeted_metabolomics_preprocessing.rst (2)

144-153: Correct container usage in IDMapper.annotate.

Initializing peptide_ids as PeptideIdentificationList aligns with annotate’s expectations.


164-169: push_back usage is appropriate.

Replacing append with push_back matches the new container API and Feature.setPeptideIdentifications accepts it.

docs/source/user_guide/identification_data.rst (2)

164-166: Good migration to PeptideIdentificationList.

Creating the container and adding via push_back is correct and keeps iteration semantics unchanged.


197-199: Loading into PeptideIdentificationList is correct.

IdXMLFile.load with pep_ids as the container is consistent with the earlier store example.

@matteopilz matteopilz linked an issue Sep 1, 2025 that may be closed by this pull request
@timosachsenberg
Copy link
Contributor

AttributeError Traceback (most recent call last)
Cell In[5], line 12
8 for i, feature_map in enumerate(feature_maps):
9 if feature_map.getMetaValue("spectra_data")[
10 0
11 ].decode() == exp.getMetaValue("mzML_path"):
---> 12 peptide_ids = oms.PeptideIdentificationList()
13 protein_ids = []
14 mapper.annotate(
15 feature_map,
16 peptide_ids,
(...)
20 exp,
21 )

@jpfeuffer
Copy link
Contributor

If you are hoping for 3.4.1: You never released it. https://pypi.org/project/pyopenms/#history
If you are hoping for nightly. We have still not adapted a branching workflow here, so we only support docs for latest stable.

@timosachsenberg
Copy link
Contributor

ah damn... forgot that we don't have a branching workflow. thanks for pointing this out

@timosachsenberg timosachsenberg modified the milestones: 3.4, 3.5 Sep 2, 2025
timosachsenberg pushed a commit that referenced this pull request Dec 15, 2025
The CI failure in PR #480 was caused by broken indentation in
interactive_plots.rst. The hd.dynspread(...) block was at column 0
instead of being indented with 4 spaces, placing it outside the
RST code-block directive.

When notebooks are generated from this RST file, the unindented code
becomes markdown text instead of Python code, causing notebook
execution to fail.

This commit applies the PR #480 changes (PeptideIdentificationList,
get2DPeakDataLong 5th arg, refactored opts) with correct indentation.
timosachsenberg added a commit that referenced this pull request Dec 15, 2025
* Fix RST indentation bug in interactive_plots.rst for PR #480

The CI failure in PR #480 was caused by broken indentation in
interactive_plots.rst. The hd.dynspread(...) block was at column 0
instead of being indented with 4 spaces, placing it outside the
RST code-block directive.

When notebooks are generated from this RST file, the unindented code
becomes markdown text instead of Python code, causing notebook
execution to fail.

This commit applies the PR #480 changes (PeptideIdentificationList,
get2DPeakDataLong 5th arg, refactored opts) with correct indentation.

* Remove broken setIntensityRange call in interactive_plots.rst

The DRange1 API changed in pyopenms 3.5.0 and no longer accepts
DPosition1 objects in its constructor. The setIntensityRange call
was causing a runtime error:

  Exception: can not handle type of (DPosition1, DPosition1)

Since intensity range filtering is optional for this visualization,
the simplest fix is to remove the call entirely. This also removes
the now-unused 'import sys' statement.

* Use ThresholdMower for intensity filtering in interactive_plots.rst

The DRange1(DPosition1, DPosition1) constructor is broken in pyopenms
3.5.0, causing setIntensityRange to fail with:

  Exception: can not handle type of (DPosition1, DPosition1)

This is a workaround that uses ThresholdMower to filter peaks with
intensity < 5000 after loading, achieving the same effect as the
original setIntensityRange call.

---------

Co-authored-by: Claude <noreply@anthropic.com>
@timosachsenberg timosachsenberg merged commit e61c85f into OpenMS:master Dec 15, 2025
7 checks passed
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.

Update doc to use new PeptideIdentificationList

3 participants