Skip to content

Conversation

@calvinp0
Copy link
Member

@calvinp0 calvinp0 commented Jan 9, 2026

Since RMG-Py has updated to 3.3.0, and AuoTST has not had many updates in the last few years, this PR attempts to bring it to the same Python as RMG-Py (3.9).

Additionally, it appears that the symmetry of [CH3].[OH] was set as 3.0 in the tests, but when testing as an RMG Molecule and also ARCSpecies, both returned 1.0. So I have set it to 1. Furthermore, a change was made to get the Molecule from xyz however this failed in the test for the TS - so the fall back now is to do from SMILES (the original code was like this).

Other minor changes were to how ASE treats Gaussian (scratch being a parameter, and force parameter not existing).

Copilot AI review requested due to automatic review settings January 9, 2026 17:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates AutoTST to Python 3.9 to align with RMG-Py 3.3.0, modernizes path handling using pathlib, and fixes several compatibility issues with updated dependencies.

Key changes:

  • Updates RMG dependency from 3.0.0 to 3.3.0 and adds explicit version pins for related packages
  • Introduces a new paths utility module to centralize path calculations using pathlib
  • Fixes ASE Gaussian calculator compatibility by using .pop("force", None) instead of del and properly handling the scratch parameter
  • Corrects symmetry number test value from 3 to 1 for [CH3].[OH] based on actual RMG behavior

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
environment.yml Updates dependency versions to match RMG 3.3.0, changes channel priorities, adds missing dependencies
autotst/utils/paths.py New utility module providing centralized path functions using pathlib
autotst/utils/init.py Adds future annotations import for new module
test/bin/g16 Updates path handling to use pathlib instead of environment variable expansion
autotst/reaction.py Fixes spelling typo and adds fallback symmetry calculation for transition states
autotst/reaction_test.py Corrects expected symmetry number value from 3 to 1
autotst/job/job.py Improves scratch directory handling to support both attribute and parameter access patterns
autotst/calculator/gaussian.py Changes from del to .pop() for safer parameter removal
autotst/data/update.py Migrates from environment variable paths to pathlib-based paths utility
autotst/data/inputoutput_test.py Updates tests to use new paths utility functions
autotst/data/base_test.py Updates tests to use new paths utility functions
autotst/calculator/vibrational_analysis_test.py Updates tests to use new paths utility functions
autotst/calculator/statmech_test.py Updates tests to use new paths utility functions
autotst/calculator/orca_test.py Updates tests to use new paths utility functions
autotst/calculator/gaussian_test.py Updates tests to use new paths utility functions
autotst/job/job_test.py Updates tests to use new paths utility functions
README.md Simplifies installation instructions by removing AUTOTST environment variable requirement

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Environment now solves for Python 3.9, as well as using the latest RMG/RMG Database. Additional packages are to ensure that the environment does not hang when importing rmgpy.data

minor fix
…pdate test setup to utilize utility functions for directory paths
@amarkpayne
Copy link
Member

I have to say, seeing a GitHub notification about an AutoTST PR did bring me some joy today! I'll yield to all of the other reviewers here, as it has been far too long since I have been familiar with this codebase. Hope all is going well for everyone here!

@amarkpayne amarkpayne removed their request for review January 9, 2026 18:05
@davidfarinajr
Copy link
Collaborator

I have to say, seeing a GitHub notification about an AutoTST PR did bring me some joy today! I'll yield to all of the other reviewers here, as it has been far too long since I have been familiar with this codebase. Hope all is going well for everyone here!

Hey Mark 👋 ! Same here 😄

Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

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

All the version changes and path changes (using pathlib) look good - thank you for doing those. We could probably use the Path class and better path handling in other areas of RMG.

But I'm cautious about the change to the TS symmetry number unit test.

Without sparing too much time to think about it, I'd have guessed H3C---H---O would have a symmetry number of 3, as we previously demanded in the test.

Looking at https://link.springer.com/content/pdf/10.1007/s00214-007-0328-0.pdf
Figure 5. It seems they say the symmetry of that TS (which is similar to this one?) is 3. (leading to a symmetry of the reaction of 4, because 12/3=4).

Image

Fortunately, we know some research groups who understand symmetry numbers better than me ;-) https://pubs.acs.org/doi/10.1021/acs.jpca.8b09024

It would be good to add a few more tests to this unit test (as the TODO notes). But at least I would like a good justification for changing the expected result from 3 to 1 (and understand why the new result is different).

It might even hint at other errors that have crept into RMG's handling of symmetry?

@calvinp0
Copy link
Member Author

All the version changes and path changes (using pathlib) look good - thank you for doing those. We could probably use the Path class and better path handling in other areas of RMG.

But I'm cautious about the change to the TS symmetry number unit test.

Without sparing too much time to think about it, I'd have guessed H3C---H---O would have a symmetry number of 3, as we previously demanded in the test.

Looking at https://link.springer.com/content/pdf/10.1007/s00214-007-0328-0.pdf Figure 5. It seems they say the symmetry of that TS (which is similar to this one?) is 3. (leading to a symmetry of the reaction of 4, because 12/3=4).
Image

Fortunately, we know some research groups who understand symmetry numbers better than me ;-) https://pubs.acs.org/doi/10.1021/acs.jpca.8b09024

It would be good to add a few more tests to this unit test (as the TODO notes). But at least I would like a good justification for changing the expected result from 3 to 1 (and understand why the new result is different).

It might even hint at other errors that have crept into RMG's handling of symmetry?

Thanks Richard! I’m not very well versed in symmetry, so I’ll need to look more carefully at the paper to fully understand it.

I was wondering, since the RMG-Py package comes with symmetry binary, would that be a possible option? I see it is used in RMG-Py here, so I could port that over? But I am not sure if is suitable for TSs?

@calvinp0
Copy link
Member Author

calvinp0 commented Jan 11, 2026

All the version changes and path changes (using pathlib) look good - thank you for doing those. We could probably use the Path class and better path handling in other areas of RMG.
But I'm cautious about the change to the TS symmetry number unit test.
Without sparing too much time to think about it, I'd have guessed H3C---H---O would have a symmetry number of 3, as we previously demanded in the test.
Looking at https://link.springer.com/content/pdf/10.1007/s00214-007-0328-0.pdf Figure 5. It seems they say the symmetry of that TS (which is similar to this one?) is 3. (leading to a symmetry of the reaction of 4, because 12/3=4).
Image
Fortunately, we know some research groups who understand symmetry numbers better than me ;-) https://pubs.acs.org/doi/10.1021/acs.jpca.8b09024
It would be good to add a few more tests to this unit test (as the TODO notes). But at least I would like a good justification for changing the expected result from 3 to 1 (and understand why the new result is different).
It might even hint at other errors that have crept into RMG's handling of symmetry?

Thanks Richard! I’m not very well versed in symmetry, so I’ll need to look more carefully at the paper to fully understand it.

I was wondering, since the RMG-Py package comes with symmetry binary, would that be a possible option? I see it is used in RMG-Py here, so I could port that over? But I am not sure if is suitable for TSs?

So, I did try symmetry on the TS (optimised at wb97xd/def2tzvp) of [CH3] + [OH], but it came up with 1.

6   0.11018300  -0.06952200  -0.44669700
1   1.16369300  -0.15632300  -0.22844900
1  -0.55458300   0.31581600   0.50496700
1  -0.36006300  -0.86675500  -1.00194300
8  -0.76434800  -0.15083600   1.34378500
1  -0.25676800   0.97818700  -0.68553900

Whilst, for the C + [H] TS (optimised same LoT), it returned 3 like the paper (but RMG returns 1 I believe from smiles of C.[H])

1     0.000000    0.000000   -2.062500
1     0.000000    0.000000   -1.158500
6     0.000000    0.000000    0.224800
1     0.000000    1.054600    0.478800
1    -0.913300   -0.527300    0.478800
1     0.913300   -0.527300    0.478800

I also attempted to do a C + [H] <> [CH3] + [H][H] test, where the expected symm is 3. However, even if we defer to symmetry to calculate the symmetry, it says 1. And looking at the XYZ from the AutoTST result, I guess it makes sense it sees it as 1.

6
TS for C+[H]_[CH3]+[H][H] (AutoTST Gen)
C  -0.410100  0.007500  0.005400
H   0.926200 -0.195700 -0.107500
H  -0.675900  0.133100  1.075100
H  -0.693900  0.921400 -0.555900
H  -0.953400 -0.865700 -0.410600
H   1.807000 -0.000600 -0.006500

Either way, I think using the TS xyz to get the RMG Molecule (using from_xyz) doesn't work as I have attempted it but RMG either returns an AtomTypeError, and if false to raise an exception, it still cannot update it. I am not sure if the prev. versions of RMG could but the current iteration appears it cannot. Unless I am missing something. So it seems maybe the best course of action is to use symmetry binary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants