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 58.81% 58.78% -0.03%
==========================================
Files 97 97
Lines 29355 29417 +62
Branches 7791 7799 +8
==========================================
+ Hits 17264 17292 +28
- Misses 9885 9915 +30
- Partials 2206 2210 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kfir4444
force-pushed
the
atom_mapping_n
branch
from
0efe1ce to
52d3fca
Compare
…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.
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.
…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.
kfir4444
force-pushed
the
atom_mapping_n
branch
from
52d3fca to
0c62c03
Compare
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
1 participant
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)