-
Notifications
You must be signed in to change notification settings - Fork 73
Updated Docs to use PeptideIdentificationList #480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR updates documentation examples across multiple user guide files to replace Python list initialization ( Changes
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
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (1)
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. Comment |
There was a problem hiding this 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.
📒 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.
|
AttributeError Traceback (most recent call last) |
|
If you are hoping for 3.4.1: You never released it. https://pypi.org/project/pyopenms/#history |
|
ah damn... forgot that we don't have a branching workflow. thanks for pointing this out |
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.
* 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>
Summary by CodeRabbit
New Features
PeptideIdentificationListas a standard container for managing peptide identifications in the public API.Documentation
Chores
jupyter_bokehdependency to requirements.✏️ Tip: You can customize this high-level summary in your review settings.