-
Notifications
You must be signed in to change notification settings - Fork 25
Seed the flipped reaction with a family so the reverse-discovery retry can map it #1023
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,7 +61,7 @@ def try_mapping(r: ARCReaction) -> list[int] | None: | |
| except ValueError: | ||
| return None | ||
| if flip: | ||
| raw_map = try_mapping(rxn.flip_reaction()) | ||
| raw_map = try_mapping(prepare_flipped_reaction(rxn)) | ||
| if raw_map is None: | ||
| return None | ||
| return check_atom_map_and_return(flip_map(raw_map)) | ||
|
|
@@ -80,6 +80,52 @@ def try_mapping(r: ARCReaction) -> list[int] | None: | |
| return check_atom_map_and_return(raw_map) | ||
|
|
||
|
|
||
| def prepare_flipped_reaction(rxn: ARCReaction) -> ARCReaction: | ||
| """ | ||
| Build the flipped reaction and give it a usable, forward-discovered family template. | ||
|
|
||
| ``ARCReaction.flip_reaction`` resets the family, so the flipped copy re-derives its product dictionaries | ||
| lazily with the *default* family set. When the original reaction was matched only by a broader set - the | ||
| usual situation for a template discovered in reverse - that rederivation comes back empty and the flip | ||
| retry has nothing to map with. Widen the search in that case, and prefer a template that describes the | ||
| flipped reaction forward. | ||
|
|
||
| Args: | ||
| rxn (ARCReaction): The reaction to flip. | ||
|
|
||
| Returns: | ||
| ARCReaction: The flipped reaction, seeded with a family and product dictionaries where possible. | ||
| """ | ||
| flipped = rxn.flip_reaction() | ||
| try: | ||
| product_dicts = flipped.product_dicts or list() | ||
| except (ValueError, KeyError, AttributeError): | ||
| product_dicts = list() | ||
| if all(pd.get('discovered_in_reverse') for pd in product_dicts): | ||
| try: | ||
| widened = flipped.get_product_dicts(rmg_family_set='all') | ||
| except (ValueError, KeyError, AttributeError): | ||
| widened = list() | ||
| logger.debug(f'Widened the family search for {flipped.label} to all families, ' | ||
| f'got {len(widened)} product dictionaries.') | ||
| product_dicts = widened or product_dicts | ||
| forward_dicts = [pd for pd in product_dicts if not pd.get('discovered_in_reverse')] | ||
| product_dicts = forward_dicts or product_dicts | ||
| if product_dicts: | ||
| # Keep only the chosen family's dictionaries. get_reaction_family_products() concatenates matches | ||
| # across every family without grouping them, while ``family`` can only name one, so leaving the | ||
| # whole list in place lets map_rxn's product_dict_index retry pair one family's recipe with | ||
| # another's label map. Every family labels its atoms *1/*2/*3, so that mispairing resolves against | ||
| # the wrong atoms and still yields a permutation, which is all check_atom_map_and_return verifies. | ||
| # TODO: replace with ARCReaction.restrict_product_dicts_to_family() once #978 lands. | ||
| family = product_dicts[0]['family'] | ||
| product_dicts = [pd for pd in product_dicts if pd['family'] == family] | ||
| flipped.product_dicts = product_dicts | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Both use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, and thank you — this is a genuine defect, not a nit. prepare_flipped_reaction assigns the whole list but takes family from product_dicts[0], so map_rxn's pdi retry can pair one family's recipe with another's r_label_map, and since both use *1/*2/*3 it succeeds against the wrong atoms exactly as you describe. Measured over the 452-reaction corpus: 7 of 335 flipped reactions carry product_dicts spanning more than one family — e.g. C10H11 <=> C10H11-2 matching Intra_Diels_alder_monocyclic + Intra_R_Add_Endocyclic + Intra_R_Add_Exocyclic. |
||
| flipped.family = family | ||
| flipped.family_own_reverse = product_dicts[0]['own_reverse'] | ||
| return flipped | ||
|
|
||
|
|
||
| def check_atom_map_and_return(atom_map: list[int] | None) -> list[int] | None: | ||
| """ | ||
| Check if the atom map is valid and return it. | ||
|
|
||
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.
Could we log these at debug?
get_reaction_family_productslogs its onw skips that way so it'd match the neighbourhoo and right now a failed widening is indistinguishable from "this reaction has no family" until it surfaces as a genericcould not be atom mappedmuch later.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.
Agreed — a failed widening should be distinguishable from "no family at all". Moving to debug to match get_reaction_family_products.