Hydrolysis families and tests #770

LeenFahoum · 2024-11-13T09:34:22Z

new hydrolysis families and tests

arc/mapping/driver.py

arc/mapping/engine.py

arc/reaction/family.py

arc/reaction/reaction.py

data/families/tests/ester_hydrolysis_test.py

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

alongd

I added some minor initial comments, I think that the commits should first be organized in terms of function definitions (there's a missing """ to one of the docstrings, making other functions hard to read)

arc/species/converter.py

alongd · 2025-03-02T04:17:19Z

arc/job/adapters/ts/heuristics.py

        spc (ARCSpecies): The species to check.

    Returns:
        bool: Whether the species is water.


I think that the issue is that there's no closing """ for the docstring

alongd · 2025-03-02T04:19:20Z

arc/job/adapters/ts/heuristics.py

    return O_counter == 1 and H_counter == 2


-def get_neighbors_by_electronegativity(spc: ARCSpecies,


this commit deletes the def of another functions

alongd

Thanks, Leen!
Please see the comments I added

arc/family/arc_families_test.py

arc/job/adapters/ts/heuristics.py

arc/reaction/reaction.py

arc/common.py

arc/common_test.py

LeenFahoum · 2025-05-11T12:35:37Z

Hi Alon,
I just finished updating the code according to your comments and force-pushed the changes.
Could you please take a look and let me know if any further adjustments are needed?

carbonyl based, nitrile, and ether hydrolysis

carbonyl- based, nitrile, and ether hydrolysis

Adds the following: 1.Reaction setup and family filtering -hydrolysis(reaction): Main entry to generate TS guesses for hydrolysis reactions -get_products_and_check_families(): Retrieves hydrolysis product data and checks for carbonyl-based and ether hydrolysis families -load_hydrolysis_parameters(): Loads family-specific parameters from hydrolysis_families_parameters.yml 2.Reactant and atom index mapping -extract_reactant_and_indices(): Identifies reactants and key atomic indices (a, b, e, d, o, h1) -process_hydrolysis_reaction(): Extracts water and main reactant from reactants -is_water(): Recognizes water molecule -get_neighbors_by_electronegativity(): Ranks neighbors by effective electronegativity, returns the top-ranked and remaining neighbors, excluding a specified atom. 3.Z-matrix setup and geometric manipulation -setup_zmat_indices(): Converts XYZ indices to Z-matrix indices -process_chosen_d_indices(): Applies bond stretching and generates TS variants with or without dihedral adjustment -stretch_ab_bond(): Stretches the a–b bond based on family parameters -get_matching_dihedrals(): Identifies dihedral angles in the Z-matrix that involve a specified set of atom indices. -Add nitrile fallback second-pass with controlled dihedral adjustment - First pass attempts nitrile TSG without forcing dihedral changes - If nitrile hydrolysis is present but no nitrile TSG is formed, run an additional pass that forces dihedral adjustments - Stop immediately once at least one nitrile TSG is found OR if all possible dihedral indices were exhausted without producing a valid TSG -Use fixed factors; try original angle first, flip only if needed - Use a fixed defined list of dihedral adjustment factors (instead of previous scattered handling) - Apply these factors to modify the original angle first - Only attempt the 180° flipped angle if no TS guesses are generated from the original angle variants - Improve normalization and docstrings for clarity 4.TS generation and validation -process_family_specific_adjustments(): Applies family-specific internal coordinates and generates TS guesses -generate_hydrolysis_ts_guess(): Builds TS geometry, validates bonds, collisions, and uniqueness -check_ts_bonds(): Validates water bond geometry in TS guess -check_dao_angle(): Rejects guesses with near-linear angels for DAO angel

carbonyl-based, nitrile, and ether hydrolysis

-test_add_atoms_using_internal_coords() -Update formatting of tests and add tests -test_add_atoms_to_xyz_using_internal_coords()

-Allow setting multiple optimization methods via opt_methods -Default to ['SLSQP', 'Nelder-Mead', 'BFGS'] if not specified -Handle None values in BEST_GUESSES to prevent errors

Raise a ValueError when only one of solvation_method or solvent is defined. Both must be provided together or both must be None.

alongd

Thank you @LeenFahoum for this contribution!
Please see me comments below

alongd · 2025-12-30T19:04:16Z

arc/species/converter.py

 ANGL_PRECISION = 0.1  # rad (for both bond angle and dihedral)


+global BEST_GUESSES


This definition has no effect, please remove.
I suggest not using the BEST_GUESSES list as a global parameter.
It is only being used in add_atom_to_xyz_using_internal_coords() where it is defined, and by average_best_guesses_from_all_methods() which is called from the previous function.
instead, you can place the smaller average_best_guesses_from_all_methods function inside the larger function, and they'll both have access to the best_guesses list.

alongd · 2025-12-30T19:06:23Z

arc/job/adapters/ts/heuristics_test.py

+        methanol=self.methanol
+        rxn2 = ARCReaction(r_species=[methylformate, water], p_species=[formicacid, methanol])
+        #RXN3
+        ethylbenzoate=self.ethylbenzoate


please leave one white-space before and after each =
(PyCharm can format your code if you select it and do Ctrl+Shift+L)

alongd · 2025-12-30T19:07:28Z

arc/job/adapters/ts/heuristics_test.py

+        for i, guess in enumerate(xyz_guesses_total):
+            a, b, e, O, H1, d = guesses_indices[i]
+            xyz_str = xyz_to_str(guess)
+            print(reaction_families[i])


are these print statement needed? or are they debugging leftovers?

alongd · 2025-12-30T19:08:27Z

arc/job/adapters/ts/heuristics_test.py

+        bnx_p3=self.bnx_p3
+        rxn15=ARCReaction(r_species=[bnx_f3, water], p_species=[bnx_p3, methanol])
+
+        tested_rxn=rxn2


I understand from this that we define above many reactions, but we only use rxn2 in the tests. Is this correct?
Maybe you want to loop through all your reactions for the test?

alongd · 2025-12-30T19:09:27Z

arc/job/adapters/ts/heuristics_test.py

+            print(a, b, e, d, O, H1)
+            print(xyz_str)
+            print()
+            if reaction_families[i]== 'carbonyl_based_hydrolysis':#the parameters of ether hydrolysis are checked in the following test section


you can remove the comment. If you think it is needed, then use two empty spaces before,
e.g.: code # comment

alongd · 2025-12-30T19:55:06Z

arc/reaction/reaction.py

        self.long_kinetic_description = ''
-        self._family = None
+        if check_family_name(family):
+            self._family = family


I think this should simply be self.family = family (no underscore)
but it should come after the block of:

self._product_dicts = None self._atom_map = None

and we should add self._family = None to the above block

alongd · 2025-12-30T19:56:25Z

arc/species/converter.py

+        raise IndexError(f"Atom index {atom_index} out of range for distance matrix of size {d_matrix .shape[0]}.")
+
+    distances = [(i, d_matrix[atom_index, i]) for i in range(d_matrix.shape[0]) if i != atom_index]
+    return sorted(distances, key=lambda x: x[1])


add an empty line at the end of the file

alongd · 2025-12-30T19:58:27Z

arc/job/adapters/xtb_test.py

                               job_type='opt',
                               project='test_9',
-                               level=Level(method='xtb', solvent='water'),
+                               level=Level(method='xtb', solvent='water', solvation_method="alpb"),


what is alpb? Is it a valid method in xTB?

alongd · 2025-12-30T19:59:26Z

data/hydrolysis_families_parameters.yml

@@ -0,0 +1,49 @@
+# Configuration file for hydrolysis reaction parameters


Please add here the citation to the paper that describes where the parameters are taken from. For now, just list authors and title, once it's out we;ll add the full citation details

alongd · 2025-12-30T20:00:20Z

arc/family/family.py

        else:
            rmg_families = list(family_sets[rmg_family_set]) \
-                if isinstance(rmg_family_set, str) and rmg_family_set in family_sets else rmg_family_set
+                if isinstance(rmg_family_set, str) and rmg_family_set in family_sets else [rmg_family_set]


if this commit fixes an issue introduced by this PR, then please squash it accordingly. If it was a previous bug, then OK to leave it.

LeenFahoum requested a review from alongd November 13, 2024 09:34

github-actions bot added Module: Main Module: Mapping Module: rmgdb Module: Scheduler Module: Species labels Nov 13, 2024

github-advanced-security bot found potential problems Nov 13, 2024

View reviewed changes

LeenFahoum force-pushed the hydrolysis branch from 15076b7 to e86ce34 Compare February 6, 2025 12:37

github-actions bot added Module: Converter Module: Plotter Module: Processor labels Feb 6, 2025

github-advanced-security bot found potential problems Feb 6, 2025

View reviewed changes

alongd force-pushed the hydrolysis branch from a569e0c to 49622d7 Compare February 13, 2025 09:32

LeenFahoum force-pushed the hydrolysis branch from 49622d7 to ef9d75c Compare February 16, 2025 10:03

LeenFahoum force-pushed the hydrolysis branch from 28d0988 to 262761d Compare February 26, 2025 07:28

alongd reviewed Mar 2, 2025

View reviewed changes

LeenFahoum force-pushed the hydrolysis branch from 262761d to efd7fd8 Compare March 4, 2025 07:55

LeenFahoum force-pushed the hydrolysis branch from 9d6023d to 0f8202b Compare May 4, 2025 11:59

alongd reviewed May 7, 2025

View reviewed changes

LeenFahoum force-pushed the hydrolysis branch from 0f8202b to 175716f Compare May 11, 2025 12:27

LeenFahoum force-pushed the hydrolysis branch from 175716f to 8783f00 Compare May 11, 2025 12:42

LeenFahoum force-pushed the hydrolysis branch 2 times, most recently from 0440a42 to 2098e03 Compare November 26, 2025 09:28

LeenFahoum force-pushed the hydrolysis branch 5 times, most recently from 7df407b to b91616f Compare December 30, 2025 15:45

Minor: type hints changes in converter

4e1d200

LeenFahoum added 18 commits December 30, 2025 17:50

Exclude hidden and underscore files from ARC families list

4800c53

Added different hydrolysis arc families groups

65d3749

carbonyl based, nitrile, and ether hydrolysis

Tests: Added tests for arc families groups

2a137f9

carbonyl- based, nitrile, and ether hydrolysis

Tests: Added tests for arc families groups

d6b7ff1

carbonyl-based, nitrile, and ether hydrolysis

Tests: updated test_get_all_families()

6e08b7b

Tests: converter r, a, d constraint functions

8863b3b

Tests: Modifications and additions to converter tests

4204c30

-test_add_atoms_using_internal_coords() -Update formatting of tests and add tests -test_add_atoms_to_xyz_using_internal_coords()

Added a function to check if a reaction family is defined

5f7158b

Defined the hydrolysis families in ts_adapters_by_rmg_family

117a75c

Enable user to define a reaction family name and check its validity

7d8261d

Update add_atom_to_xyz_using_internal_coords()

9fe7e5c

-Allow setting multiple optimization methods via opt_methods -Default to ['SLSQP', 'Nelder-Mead', 'BFGS'] if not specified -Handle None values in BEST_GUESSES to prevent errors

Add function to get sorted distances from a given atom in a molecule

82bdbf3

Tests: sorted_distances_of_atom() in converter

27b3b70

Added atoms electronegativity values YAML file

eda6db1

Fix: enforce consistency between solvation method and solvent

1b9c9a6

Raise a ValueError when only one of solvation_method or solvent is defined. Both must be provided together or both must be None.

Added YAML file for hydrolysis reactions parameters

1a65a22

Fix TypeError in get_all_families when using a single family name string

b91616f

alongd reviewed Dec 30, 2025

View reviewed changes

		return O_counter == 1 and H_counter == 2


		def get_neighbors_by_electronegativity(spc: ARCSpecies,

		ANGL_PRECISION = 0.1 # rad (for both bond angle and dihedral)


		global BEST_GUESSES

		@@ -0,0 +1,49 @@
		# Configuration file for hydrolysis reaction parameters

Hydrolysis families and tests #770

Are you sure you want to change the base?

Hydrolysis families and tests #770

Conversation

LeenFahoum commented Nov 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeenFahoum commented May 11, 2025

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants