-
Notifications
You must be signed in to change notification settings - Fork 24
Optimize Atom Mapping procedure #906
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
Open
kfir4444
wants to merge
19
commits into
main
Choose a base branch
from
optimize_am
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+897
−154
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
b0c30f8
feat: add ctypes loader for pre-compiled zmat C kernels
kfir4444 38bd22c
fix: guard against collinear triplets in determine_d_atoms()
kfir4444 61176d9
perf: wire C kernel fast paths in calculate_distance and calculate_di…
kfir4444 7c0eae5
perf: skip VF2 in order_atoms_in_mol_list when IDs already match
kfir4444 c8f25a8
feat: add skip_conformers parameter to scissors() and _scissors()
kfir4444 d5c92d0
perf: replace set_dihedral() round-trip with Rodrigues rotation
kfir4444 cf0ea0c
perf: accelerate get_product_dicts via CPI cache, shared iso-caches, …
kfir4444 e9f5ab6
build: track _zmat_c_kernels.c and pre-compiled .so in git
kfir4444 c4840f9
build: add _zmat_c_kernels.so compilation step to 'make compile'
kfir4444 b23e459
perf: wire calculate_angle C kernel fast path
kfir4444 a0e38c3
tests: relax calculate_angle precision to places=2
kfir4444 63115fb
tests: update CFOUR expected angle to match float32 kernel output
kfir4444 80be6fa
fix: use RMG_PYTHON directly to avoid broken conda shim in micromamba CI
kfir4444 d9bf38c
fix: address CodeQL warnings — remove unused imports and document emp…
kfir4444 f5c8bba
fix: prefer MAMBA_EXE run to avoid arc_env library leakage into rmg_env
kfir4444 210e6f9
fix: apply MAMBA_EXE env isolation to compare_thermo and compare_rates
kfir4444 38d0479
tests: make NH3_elimination TS geometry check symmetric-map-agnostic
kfir4444 47e0b9b
tests: make linear TS geometry checks agnostic to atom-map ordering
kfir4444 245c019
tests: fix IndexError in product_dict loop — cap at actual dict count
kfir4444 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -263,7 +263,14 @@ def generate_unimolecular_products(self, | |
| """ | ||
| isomorphic_subgraph_dicts = list() | ||
| reactant_to_group_maps = reactant_to_group_maps[0] | ||
| for mol in reactants[0].mol_list or [reactants[0].mol]: | ||
| mol_list = reactants[0].mol_list or [reactants[0].mol] | ||
| # For large molecules (>20 atoms) with many resonance structures, limit the search | ||
| # to avoid combinatorial explosion. Most reaction sites are captured by the first | ||
| # few Kekulé forms; additional resonance structures produce equivalent sites already | ||
|
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. resonance doesn't always mean Kekulé / Clar type. there are many others, including complex |
||
| # covered by the adjacency-list deduplication in check_product_isomorphism. | ||
| if len(mol_list) > 2 and len(mol_list[0].atoms) > 20: | ||
| mol_list = mol_list[:2] | ||
| for mol in mol_list: | ||
| for reactant_to_group_map in reactant_to_group_maps: | ||
| group = self.groups_by_label[reactant_to_group_map['subgroup']] | ||
| isomorphic_subgraphs = mol.find_subgraph_isomorphisms(other=group, save_order=True) | ||
|
|
@@ -512,30 +519,56 @@ def get_reaction_family_products(rxn: ARCReaction, | |
| consider_rmg_families=consider_rmg_families, | ||
| consider_arc_families=consider_arc_families) | ||
| product_dicts = list() | ||
| # Pre-build the flipped reaction and copies once; guard against ValueError from to_smiles() | ||
| # (e.g. when openbabel is unavailable) so we can fall back to lazy creation per family. | ||
| try: | ||
| flipped_rxn = rxn.flip_reaction(report_family=False) | ||
| fwd_reactants, fwd_products = rxn.get_reactants_and_products(return_copies=True) | ||
| rev_reactants, rev_products = flipped_rxn.get_reactants_and_products(return_copies=True) | ||
| except (KeyError, ValueError, InvalidAdjacencyListError) as e: | ||
| logger.debug(f'Could not pre-build flipped reaction copies: {type(e).__name__}: {e}') | ||
| flipped_rxn = fwd_reactants = fwd_products = rev_reactants = rev_products = None | ||
| # Shared adjacency-list → bool caches for check_product_isomorphism: one per direction. | ||
| # Families appearing multiple times in the family list (e.g. due to 'all' including both | ||
| # RMG and ARC copies) share results for the same template-product graph across calls. | ||
| fwd_iso_cache: dict[tuple, bool] = {} | ||
| rev_iso_cache: dict[tuple, bool] = {} | ||
| for family_label in family_labels: | ||
| try: | ||
| # Forward: | ||
| products = determine_possible_reaction_products_from_family(rxn=rxn, | ||
| family_label=family_label, | ||
| consider_arc_families=consider_arc_families, | ||
| reverse=False, | ||
| reactants=fwd_reactants, | ||
| p_species=fwd_products, | ||
| iso_cache=fwd_iso_cache, | ||
| ) | ||
| if len(products): | ||
| product_dicts.extend(filter_products_by_reaction(rxn=rxn, product_dicts=products)) | ||
| product_dicts.extend(filter_products_by_reaction(rxn=rxn, product_dicts=products, | ||
| p_species=fwd_products)) | ||
| fwd_reactants = rxn.get_reactants_and_products(return_copies=True)[0] | ||
|
|
||
| # Reverse: | ||
| flipped_rxn = rxn.flip_reaction(report_family=False) | ||
| if flipped_rxn is None: | ||
| flipped_rxn = rxn.flip_reaction(report_family=False) | ||
| rev_reactants, rev_products = flipped_rxn.get_reactants_and_products(return_copies=True) | ||
| products = determine_possible_reaction_products_from_family(rxn=flipped_rxn, | ||
| family_label=family_label, | ||
| consider_arc_families=consider_arc_families, | ||
| reverse=True, | ||
| reactants=rev_reactants, | ||
| p_species=rev_products, | ||
| iso_cache=rev_iso_cache, | ||
| ) | ||
| if len(products): | ||
| filtered_products = filter_products_by_reaction(rxn=flipped_rxn, product_dicts=products) | ||
| filtered_products = filter_products_by_reaction(rxn=flipped_rxn, product_dicts=products, | ||
| p_species=rev_products) | ||
| if not discover_own_reverse_rxns_in_reverse: | ||
| product_dicts.extend([prod for prod in filtered_products if not prod['own_reverse']]) | ||
| else: | ||
| product_dicts.extend(filtered_products) | ||
| rev_reactants = flipped_rxn.get_reactants_and_products(return_copies=True)[0] | ||
| except (KeyError, ValueError, InvalidAdjacencyListError) as e: | ||
| logger.debug(f'Skipping family {family_label} due to unsupported group definition: {type(e).__name__}: {e}') | ||
| return product_dicts | ||
|
|
@@ -545,6 +578,9 @@ def determine_possible_reaction_products_from_family(rxn: ARCReaction, | |
| family_label: str, | ||
| consider_arc_families: bool = True, | ||
| reverse: bool = False, | ||
| reactants: list | None = None, | ||
| p_species: list | None = None, | ||
| iso_cache: dict | None = None, | ||
| ) -> list[dict]: | ||
| """ | ||
| Determine the possible reaction products for a given ARC reaction and a given RMG reaction family. | ||
|
|
@@ -567,20 +603,48 @@ def determine_possible_reaction_products_from_family(rxn: ARCReaction, | |
| family_label (str): The reaction family label. | ||
| consider_arc_families (bool, optional): Whether to consider ARC's custom families. | ||
| reverse (bool, optional): Whether the reaction is in reverse. | ||
| reactants (list, optional): Pre-built reactant copies to pass directly to generate_products. | ||
| When provided, skips the internal get_reactants_and_products() call. | ||
| The caller is responsible for ensuring these are fresh copies when | ||
| a previous call may have mutated them (i.e., when products were found). | ||
| p_species (list, optional): Pre-built product species copies for isomorphism check. | ||
| When provided, skips the internal get_reactants_and_products() call | ||
| inside isomorphic_products. The caller must ensure these are not mutated. | ||
| iso_cache (dict, optional): Shared adjacency-list → bool cache for check_product_isomorphism. | ||
| When provided, results are shared across multiple calls with the same | ||
| p_species (e.g., across all families in get_reaction_family_products). | ||
| The caller owns the dict and must NOT share it across different p_species. | ||
|
|
||
| Returns: | ||
| list[dict]: A list of dictionaries, each containing the family label, the group labels, the products, | ||
| and whether the family's template also represents its own reverse. | ||
| """ | ||
| product_dicts = list() | ||
| family = get_reaction_family(label=family_label, consider_arc_families=consider_arc_families) | ||
| products = family.generate_products(reactants=rxn.get_reactants_and_products(return_copies=True)[0]) | ||
| if reactants is None: | ||
| reactants = rxn.get_reactants_and_products(return_copies=True)[0] | ||
| products = family.generate_products(reactants=reactants) | ||
| if products: | ||
| if p_species is None: | ||
| p_species = rxn.get_reactants_and_products(return_copies=True)[1] | ||
| if iso_cache is None: | ||
| iso_cache = {} | ||
| for group_labels, product_lists in products.items(): | ||
| for product_list in product_lists: | ||
| template_mols, r_label_dict = product_list[0], product_list[1] | ||
| if not isomorphic_products(rxn=rxn, products=template_mols): | ||
| continue | ||
| # Cache check_product_isomorphism by adjacency-list key. | ||
| # Many generate_products outputs share the same graph (same atom connectivity | ||
| # and ordering) but carry different atom-label mappings; the isomorphism result | ||
| # depends only on the graph, so we compute it once per unique graph. | ||
| adj_key = tuple(m.to_adjacency_list() for m in template_mols) | ||
| if adj_key in iso_cache: | ||
| if not iso_cache[adj_key]: | ||
| continue | ||
| else: | ||
| result = check_product_isomorphism(products=template_mols, p_species=p_species) | ||
| iso_cache[adj_key] = result | ||
| if not result: | ||
| continue | ||
| # Build r_label_map preserving duplicate labels by suffixing | ||
| # (e.g., R_Recombination has two atoms labeled '*' → '*' and '*_2'). | ||
| r_label_map = {} | ||
|
|
@@ -621,6 +685,7 @@ def determine_possible_reaction_products_from_family(rxn: ARCReaction, | |
|
|
||
| def filter_products_by_reaction(rxn: ARCReaction, | ||
| product_dicts: list[dict], | ||
| p_species: list | None = None, | ||
| ) -> list[dict]: | ||
| """ | ||
| Filter the possible reaction products by the ARC reaction. | ||
|
|
@@ -629,12 +694,15 @@ def filter_products_by_reaction(rxn: ARCReaction, | |
| rxn (ARCReaction): The ARC reaction object. | ||
| product_dicts (list[dict]): A list of dictionaries, each containing the family label, the group labels, | ||
| the products, and whether the family's template also represents its own reverse. | ||
| p_species (list, optional): Pre-built product species copies. When provided, skips the internal | ||
| get_reactants_and_products() call. The caller must ensure these are not mutated. | ||
|
|
||
| Returns: | ||
| list[dict]: The filtered list of product dictionaries. | ||
| """ | ||
| filtered_product_dicts, r_label_maps = list(), list() | ||
| _, p_species = rxn.get_reactants_and_products(return_copies=True) | ||
| if p_species is None: | ||
| _, p_species = rxn.get_reactants_and_products(return_copies=True) | ||
| for product_dict in product_dicts: | ||
| if len(product_dict['products']) != len(p_species): | ||
| continue | ||
|
|
||
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
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
Oops, something went wrong.
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.
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.
I agree with the copilot comment. On a different x86-64 CPU with an older microarchitecture you could get SIGILL that kills Python. let's rethink it