Fix mapping reverse discovery - #935
Open
calvinp0 wants to merge 3 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #935 +/- ##
==========================================
- Coverage 63.54% 63.51% -0.03%
==========================================
Files 114 114
Lines 38363 38401 +38
Branches 10033 10043 +10
==========================================
+ Hits 24377 24392 +15
- Misses 11066 11087 +21
- Partials 2920 2922 +2
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:
|
For a reaction whose family was discovered in the reverse direction, the
first-orientation map_rxn attempt fails on get_template_product_order (or on a
missing template map) and returns None, after which map_reaction routinely
recovers via its existing flip fallback and returns a valid map. The two
intermediate logger.error calls in map_rxn therefore fired as false alarms on
reactions that mapped successfully.
Downgrade both to logger.debug. Control flow is unchanged (both branches still
return None as before), so the returned atom map is identical for every
reaction. A genuine total-mapping failure (both orientations exhausted) is
still surfaced at error level by ARCReaction.atom_map ("could not be atom
mapped").
Add a focused test that drives the first-orientation failure via map_rxn and
asserts no ERROR is logged, while map_reaction still returns a valid map.
_select_ch3_anchors() promised a second anchor B forming a non-linear B-X-A angle, but never tested for colinearity and never fell back to hydrogens. For a linear heavy-atom skeleton (e.g., propyne, CH3-C#CH) every heavy candidate is colinear with the X->A axis, so _construct_local_axes() raised 'Anchors are colinear; cannot define unique plane.', map_rxn() returned None, and the whole reaction could not be atom mapped (surfaced by a Retroene benchmark reaction, C=C=CCC <=> C#CC + C=C, once the species carried optimized geometries). Now anchor selection walks candidates in priority order (heavy neighbor of A, any other neighbor of A, the XH3 hydrogens, any remaining atom) and returns the first that passes the same non-colinearity criterion _construct_local_axes() enforces, so selection can never hand it anchors it will reject. _construct_local_axes() itself remains strict. Also, _map_xh3_group() now rejects a (None, None) anchor tuple (a non-empty tuple is truthy, so the previous guard missed it) and returns None instead of propagating a ValueError, and map_hydrogens() falls back to an in-order assignment of the three equivalent XH3 hydrogens when the geometric refinement fails, so the resulting map stays a complete permutation.
calvinp0
force-pushed
the
fix_mapping_reverse_discovery
branch
from
August 2, 2026 20:22
ba734b2 to
44a2d9e
Compare
kfir4444
reviewed
Aug 3, 2026
| return None | ||
|
|
||
|
|
||
| def _anchors_define_a_plane(center_index: int, |
Collaborator
There was a problem hiding this comment.
Ideally, this would be imported from arc.species.vectors for standardization. The implementation issue is that we don't have an entryway function in the calculate_dihedral pipeline that takes in 3 vectors and outputs the dihedral angle. I suggest adding a function to arc/species/vectors.py (call it get_vector_dihedral or something), then change the get_dihedral to use it after creating the 3 vectors, and import it here too. This is the best method to ensure SOTA computations.
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
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 pull request makes several improvements to the atom mapping logic in the
arc.mappingmodule, particularly for reactions involving XH₃ (e.g., CH₃) groups and for reactions discovered in the reverse direction. The changes focus on making the mapping more robust for linear molecules, ensuring that recoverable mapping failures do not log errors unnecessarily, and improving test coverage for these scenarios.Mapping logic improvements
_select_ch3_anchorsinengine.pyto robustly select non-colinear anchors for XH₃ groups, handling linear molecules by falling back to hydrogens and ensuring a valid plane can always be defined. Introduced_anchors_define_a_planeto encapsulate the colinearity check._map_xh3_groupto returnNoneif a local frame cannot be constructed, and mademap_hydrogensfall back to assigning hydrogens in order when geometric refinement fails, ensuring a complete atom map is always produced. [1] [2] [3]Improved error handling and logging
map_rxnso that recoverable failures (e.g., due to orientation issues in reactions discovered in reverse) are logged atDEBUGlevel instead ofERROR, preventing false alarms for benign failures.try_mappingto immediately attempt the flip fallback for reactions discovered in reverse, improving efficiency and clarity.Test enhancements
engine_test.pyto verify anchor selection for linear molecules, ensure that mapping gracefully handles undefined planes, and confirm robust mapping for Retroene and other challenging reactions.driver_test.pyto check that recoverable failures do not log errors and that retro-Diels-Alder reactions are mapped correctly in the discovered direction.engine_test.py.These changes collectively make the atom mapping more robust, especially for edge cases involving linear molecules and reactions with reverse-discovered families, while also improving test coverage and reducing noise from unnecessary error logs.