fix: fix Mo when O is also contained #2320
Conversation
Fix #2318. Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
📝 WalkthroughWalkthroughThe pull request introduces modifications to the regular expression handling in the Changes
Assessment against linked issues
The changes appear to directly address the issue of incorrectly recognizing the Mo atom in SMILES representation by modifying the regular expression pattern to handle element wrapping more comprehensively. Sequence DiagramsequenceDiagram
participant Path Generator
participant Regex Handler
participant SMILES Converter
Path Generator->>Regex Handler: Apply modified regex pattern
Regex Handler-->>Path Generator: Process element wrapping
Path Generator->>SMILES Converter: Convert with improved regex
The sequence diagram illustrates the enhanced regex handling process for element recognition, particularly focusing on the Mo atom case mentioned in the issue. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2320 +/- ##
=======================================
Coverage 96.19% 96.19%
=======================================
Files 17 17
Lines 1418 1418
=======================================
Hits 1364 1364
Misses 54 54 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_reacnetgen.py (1)
162-167: Add more coverage for bracketed metals.While this test effectively confirms that
[Mo]is recognized, consider also testing other patterns like"[Mo]O","[Mo][O]", or embedded forms to ensure the new regex logic is robust for edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
reacnetgenerator/_path.py(1 hunks)tests/test_reacnetgen.py(1 hunks)
🔇 Additional comments (2)
tests/test_reacnetgen.py (1)
155-161: Confirm that transitioningatomnameto a NumPy array won’t break downstream logic.This change to use a NumPy array is acceptable and works fine for the subsequent
_recalls in this test. However, ensure that other parts of the code relying onatomnamedo not expect a plain list (e.g., slicing or appending).reacnetgenerator/_path.py (1)
250-250: Validate broadened regex boundaries.The updated lookahead
(?!H|\])addresses the issue of elements immediately followed by a closing bracket, but consider verifying collisions between multi-letter elements (e.g., “Mo” vs. “O”). Additional tests may detect unintended bracket expansions.
CodSpeed Performance ReportMerging #2320 will not alter performanceComparing Summary
|
Fix #2318.
Summary by CodeRabbit
New Features
Bug Fixes
convertSMILESmethod to provide better exception contextTests