-
Notifications
You must be signed in to change notification settings - Fork 70
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughDocumentation examples were updated to use oms.PeptideIdentificationList instead of Python lists, with corresponding size/count and push_back adjustments. One GNPS example now uses an explicit size check. Interactive plotting code adds a fifth argument to get2DPeakDataLong and simplifies opts usage. requirements.txt adds jupyter_bokeh. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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 |
Summary by CodeRabbit
New Features
Documentation
Chores