-
Notifications
You must be signed in to change notification settings - Fork 18
AutoTST Update (Python 3.9) #96
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?
Conversation
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.
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 ofdeland 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.
89bed85 to
763081e
Compare
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
… and test data directories
…improve path handling minor fix minor fix
…pdate test setup to utilize utility functions for directory paths
|
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 😄 |
rwest
left a comment
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.
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).
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 |
So, I did try Whilst, for the C + [H] TS (optimised same LoT), it returned 3 like the paper (but RMG returns 1 I believe from smiles of I also attempted to do a C + [H] <> [CH3] + [H][H] test, where the expected symm is 3. However, even if we defer to Either way, I think using the TS xyz to get the RMG Molecule (using |

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).