-
Notifications
You must be signed in to change notification settings - Fork 0
Bugfixes #40
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
Bugfixes #40
Conversation
WalkthroughUpdated the openms-streamlit-vue-component submodule reference to a new commit and added selenocysteine ('U') with mass 150.953633405 to the amino acid mass table in src/render/sequence.py. No changes to functions, control flow, or public APIs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
🔭 Outside diff range comments (1)
src/render/sequence.py (1)
266-273: Fix: 'cy' → 'cz' (ion-type mismatch in internal fragment code)Short: Verified — the codebase uses 'cz' elsewhere but the loop at src/render/sequence.py iterates ['by','bz','cy']. getInternalFragmentMassesWithSeq and other places explicitly check/expect 'cz', so the current 'cy' will take the wrong branch and produce incorrect shifts/fragment masses.
Locations to fix (found via repo search):
- src/render/sequence.py:40-42 — branch checks
res_type == 'cz'- src/render/sequence.py:108-110 — earlier loop uses
['ax', 'by', 'cz']- src/render/sequence.py:203-206 — getInternalFragmentMassesWithSeq:
shift = ... res_type == 'cz'- src/render/sequence.py:211-214 — tmp code:
czp, czs = getFragmentMassesWithSeq(..., 'cz')- src/render/sequence.py:266-273 — current loop (the problematic snippet):
- for ion_type in ['by', 'bz', 'cy']: # by cz are the same.
Suggested fixes (choose one):
Non-breaking (recommended if consumers rely on 'cy' keys in output):
Replace the call with an internal mapping so output keys remain "cy" but computation uses "cz":for ion_type in ['by', 'bz', 'cy']: internal_type = 'cz' if ion_type == 'cy' else ion_type ions, start_indices, end_indices = getInternalFragmentMassesWithSeq( sequence, internal_type, modifications ) out_object[f'fragment_masses_{ion_type}'] = ions ...Simple/safe fix (if you can change output keys):
- for ion_type in ['by', 'bz', 'cy']: + for ion_type in ['by', 'bz', 'cz']:This is a functional bug — please apply one of the fixes above.
🧹 Nitpick comments (3)
src/render/sequence.py (3)
215-216: Nit: typos and naming (“termianl” → “terminal”, comments spelling)Renaming improves readability, and comment spelling fixes reduce friction for future maintenance. No behavioral changes.
- termianl_masses = byp + bys + czp + czs#tmp code - termianl_masses.sort()#tmp code + terminal_masses = byp + bys + czp + czs # tmp code + terminal_masses.sort() # tmp code- # Only conside fragments of at least length 5 + # Only consider fragments of at least length 5- # TODO: Should be adressed in component - if isMatchWithTolerance(termianl_masses, mass, 10.0): #tmp code + # TODO: Should be addressed in component + if isMatchWithTolerance(terminal_masses, mass, 10.0): # tmp codeAlso applies to: 233-233, 248-249
253-253: Nit: use the H20 constant instead of a hardcoded literalKeeps constants centralized and avoids magic numbers.
- masses.append(mass + 18.010564683 + shift) + masses.append(mass + H20 + shift)
165-167: Align selenocysteine mass precision and document sourceTo keep all entries at six-decimal precision and cite the upstream definition, update in
src/render/sequence.py:• Line 166:
- 'U': 150.953633405, + 'U': 150.953633, # source: Unimod (mod 162)/OpenMS monoisotopic mass 150.9536334 Da
📜 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 ignored due to path filters (3)
js-component/dist/assets/index-7cc3fbf2.cssis excluded by!**/dist/**js-component/dist/assets/index-871c0fa8.jsis excluded by!**/dist/**js-component/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (2)
openms-streamlit-vue-component(1 hunks)src/render/sequence.py(1 hunks)
🔇 Additional comments (1)
openms-streamlit-vue-component (1)
1-1: Ensure CI Can Fetch the Submodule and Verify Host-App ContractsI ran the recommended submodule-init script and saw clone failures due to SSH not being available in this environment. Please:
- Update the submodule remote in
.gitmodulesto use an HTTPS URL (or ensure your CI agents have SSH set up and keys loaded).- Add a step in your CI config to run:
before any build or test commands.git submodule update --init --recursive- Double-check that the bumped commit in openms-streamlit-vue-component doesn’t introduce breaking UI prop or event name changes in the host Streamlit integration.
Bidirectional Mass Selection for Fragment Table
Filtering stays applied if table content is changed
Add selenocystein
Summary by CodeRabbit
New Features
Chores