Several more atom mapping improvements#839
Open
kfir4444 wants to merge 15 commits into
Open
Conversation
Collaborator
Author
|
Added a few more commits:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #839 +/- ##
==========================================
- Coverage 60.68% 60.65% -0.04%
==========================================
Files 103 103
Lines 31186 31254 +68
Branches 8128 8138 +10
==========================================
+ Hits 18926 18956 +30
- Misses 9910 9927 +17
- Partials 2350 2371 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kfir4444
force-pushed
the
atom_mapping_n
branch
from
0efe1ce to
52d3fca
Compare
kfir4444
force-pushed
the
atom_mapping_n
branch
from
52d3fca to
0c62c03
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ns and multiplicity When a bond in a cyclic molecule is broken (ring-opening), the resulting fragment must have new radical electrons assigned to the atoms that were part of the severed bond. This commit fixes _scissors() to: 1. Detect ring-opening cases (len(mol_splits) == 1). 2. Assign radical electrons to the cut atoms based on their missing valency. 3. Update the fragment's multiplicity. 4. Set keep_mol=True and provide final_xyz to the new ARCSpecies to ensure proper initialization. This prevents ValueError and SpeciesError during molecule perception when mapping reactions involving ring-opening/closing.
Added a check for None results from cut_species_based_on_atom_indices in map_rxn. If scission fails for reactants or products, the function now attempts to fall back to the next dictionary template (if available) or returns None with a logged error, preventing a TypeError when calling update_xyz on a None object.
Updated benzene scission tests to allow for multiple valid atom mappings due to the symmetry of the molecule. Replacing assertEqual with assertIn ensures the tests pass if any of the chemically equivalent valid maps are returned. This was also verified with the NEB, achieving the same TS with both AMs.
Collaborator
Author
|
The previous error in CI was caused by |
alongd
reviewed
Jun 7, 2026
alongd
left a comment
alongd
left a comment
Member
There was a problem hiding this comment.
Thanks! Please see some comments
| pairs.append((react, prod)) | ||
| p_cuts.pop(idx) | ||
| found = False | ||
| for res in res1: |
Member
There was a problem hiding this comment.
can res1 be None? Shall we protect against it?
Member
There was a problem hiding this comment.
I think this comment wasn't addressed. take a look?
Collaborator
Author
There was a problem hiding this comment.
I think it was see L#1193
| from typing import TYPE_CHECKING | ||
| from __future__ import annotations | ||
|
|
||
| from typing import Dict, List, Optional, Tuple, TYPE_CHECKING |
Member
There was a problem hiding this comment.
We don't need those in Python 3.14. remove (just keep TYPE_CHECKING as in the original) and rebuild the env?
Collaborator
Author
There was a problem hiding this comment.
I need to read more about the new way python deals with typing. For now, I'm reverting these.
This commit introduces several optimizations to the atom mapping algorithm: 1. identify_superimposable_candidates: Reduced algorithmic complexity from O(N^2) to O(N) by starting graph traversal from only the first heavy atom. For connected molecular graphs, this is sufficient to find all valid mappings (including symmetries) and significantly reduces redundant DFS calls. 2. prune_identical_dicts: Fixed a logic bug where dictionaries were incorrectly pruned if they shared a single key-value pair. Now correctly uses exact dictionary equality. 3. pairing_reactants_and_products_for_mapping: Pre-calculates resonance structures for all reactant fragments once before the pairing loop, avoiding redundant O(R*P) computations. 4. copy_species_list_for_mapping: Replaced the expensive spc.copy() (which uses dictionary serialization) with a lighter direct ARCSpecies instantiation to reduce overhead during the mapping process. safe generate_resonance_structures_safely
…erception Modified ARCSpecies.__init__ to skip the expensive mol_from_xyz() call (which performs molecule perception from 3D coordinates) when a valid Molecule object is already provided and the keep_mol flag is set. This significantly reduces initialization overhead during scission and atom mapping operations where the molecular graph is already known.
To facilitate new scissors logic
- Implement global ReactionFamily caching to avoid redundant instantiation. - Refactor ReactionFamily initialization to selectively load only leaf-level entries identified from the template, significantly reducing object creation overhead. - Improve RMG group file parsing robustness and performance using pre-compiled regex and recursive entry extraction for OR complexes. - Refine product generation logic to use pre-loaded groups, avoiding repeated parsing and object creation during mapping.
alongd
reviewed
Jun 8, 2026
alongd
reviewed
Jun 8, 2026
| break | ||
| return specific_entries | ||
| groups_str = "\n" + "".join(groups_as_lines) | ||
| # Split by entry( but keep the delimiter-ish part |
Member
There was a problem hiding this comment.
see the comment, seems broken? "("
Collaborator
Author
There was a problem hiding this comment.
No, that's the regex expression (L942)
…on families - Fix generate_bimolecular_products to correctly handle swapped reactant orders relative to the family template while maintaining original molecule sequence for product generation. - Fix descent_complex_group to robustly split OR complexes by stripping whitespace from labels. - Fix typo logger.Error -> logger.error in ARCReaction multiplicity property. - Refine selective entry loading to traverse and preserve all necessary intermediate nodes for OR complex resolution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves several aspects of atom mappings.
two key fixes:
ARCSpecies._scissors. This is a major issue that improves both accuracy and performence:Moreover, this is critical for cyclic species, since debugging showed we did not include the original XYZ in the ARCSpecies, which caused errors.
identify_superimposable_candidates, which was being calledOther fixes added, and more generalization of AM testing (manually verified)