Skip to content

Conversation

LeenFahoum
Copy link
Contributor

new hydrolysis families and tests

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

spc (ARCSpecies): The species to check.
Returns:
bool: Whether the species is water.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return O_counter == 1 and H_counter == 2


def get_neighbors_by_electronegativity(spc: ARCSpecies,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this commit deletes the def of another functions

Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Leen!
Please see the comments I added

@LeenFahoum
Copy link
Contributor Author

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?

alongd and others added 8 commits May 11, 2025 15:43
ester, nitrile, ether, and imine hydrolysis
ester, nitrile, ether,, and imine 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 ester 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, f, 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.

    -generate_dihedral_variants(): Generates dihedral angle variants for near 0 and near 180 dihedrals

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
ester, nitrile, ether,, and imine hydrolysis
LeenFahoum added 18 commits May 11, 2025 15:43
-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
-test_add_atoms_using_internal_coords()

-Update formatting of tests and add tests

-test_add_atoms_to_xyz_using_internal_coords()
Raise a ValueError when only one of solvation_method or solvent is defined.
Both must be provided together or both must be None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants