Conversation
There was a problem hiding this comment.
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.
calvinp0
force-pushed
the
autotst_update
branch
from
89bed85 to
763081e
Compare
|
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
rwest
left a comment
There was a problem hiding this comment.
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 |
|
Hi @rwest , sorry to bug you again about this branch. I reverted the change of the reaction test as I did investigating and optimisations and did fine it was a sym of 3. However, since AutoTST is rather a TS guesser, I believe it shouldn't really be trying to provide the symmetry until a TS has been optimised which in this test it wasn't. I am though going to leave it as the original developers intended. If the rest of the changes look good, I would like to merge it so that it works with RMG 3.9 and that our team can continue to use it without needing to switch branches. Thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
autotst/job/job.py
Outdated
| ase_calculator.write_input(conformer.ase_molecule) | ||
| try: | ||
| os.makedirs(ase_calculator.scratch) | ||
| except OSError: | ||
| pass | ||
| scratch = getattr(ase_calculator, "scratch", None) or ase_calculator.parameters.get("scratch") | ||
| if not scratch: | ||
| raise AttributeError("ASE Gaussian calculator has no scratch location (no .scratch and no parameters['scratch']).") | ||
| os.makedirs(scratch, exist_ok=True) | ||
|
|
||
| shutil.move( | ||
| ase_calculator.label + ".com", | ||
| os.path.join( | ||
| ase_calculator.scratch, | ||
| ase_calculator.label + ".com" | ||
| )) | ||
| base = ase_calculator.label | ||
| com_src = f"{base}.com" | ||
| if not os.path.exists(com_src): | ||
| raise FileNotFoundError(f"Expected ASE to write {com_src} in CWD={os.getcwd()}") | ||
|
|
||
| shutil.move( | ||
| ase_calculator.label + ".ase", | ||
| com_src, | ||
| os.path.join( | ||
| ase_calculator.scratch, | ||
| ase_calculator.label + ".ase" | ||
| scratch, | ||
| com_src | ||
| )) |
There was a problem hiding this comment.
This now assumes ASE always writes {label}.com into the current working directory. Some ASE Gaussian implementations/configurations write directly into the scratch/label path; in that case this will raise FileNotFoundError even though the input exists. A more robust approach is to check both com_src in CWD and os.path.join(scratch, com_src) (or to ask ASE for the actual written path if available) before failing.
autotst/job/job.py
Outdated
| scratch = getattr(ase_calculator, "scratch", None) or ase_calculator.parameters.get("scratch") | ||
| if not scratch: | ||
| raise AttributeError("ASE Gaussian calculator has no scratch location (no .scratch and no parameters['scratch']).") | ||
| file_path = os.path.join(scratch, label) |
There was a problem hiding this comment.
Scratch-resolution logic is duplicated in multiple methods (write_input, submit_conformer, submit_transitionstate, submit_rotor). Consider centralizing this into a small helper (e.g., _get_ase_scratch(ase_calculator)) to keep behavior consistent and reduce future drift when ASE changes again.
| def calculate_symmetry_number(self): | ||
| try: | ||
| return super(TS, self).calculate_symmetry_number() | ||
| except rmgpy.exceptions.AtomTypeError: | ||
| logging.info("AtomTypeError in symmetry calculation (likely TS bond). Using RMG molecule graph for symmetry.") | ||
| if self.rmg_molecule: | ||
| try: | ||
| species = rmgpy.species.Species(molecule=[self.rmg_molecule]) | ||
| self._symmetry_number = species.get_symmetry_number() | ||
| except Exception: | ||
| self._symmetry_number = self.rmg_molecule.get_symmetry_number() | ||
| else: | ||
| raise | ||
| return self._symmetry_number |
There was a problem hiding this comment.
Catching a bare Exception here can unintentionally mask real issues (e.g., programming errors, unexpected RMG API changes) and silently change symmetry behavior. Prefer narrowing the exception type(s) you expect from species.get_symmetry_number(), and/or log the exception details before falling back to self.rmg_molecule.get_symmetry_number().
| def calculate_symmetry_number(self): | ||
| try: | ||
| return super(TS, self).calculate_symmetry_number() | ||
| except rmgpy.exceptions.AtomTypeError: | ||
| logging.info("AtomTypeError in symmetry calculation (likely TS bond). Using RMG molecule graph for symmetry.") | ||
| if self.rmg_molecule: | ||
| try: | ||
| species = rmgpy.species.Species(molecule=[self.rmg_molecule]) | ||
| self._symmetry_number = species.get_symmetry_number() | ||
| except Exception: | ||
| self._symmetry_number = self.rmg_molecule.get_symmetry_number() | ||
| else: | ||
| raise | ||
| return self._symmetry_number |
There was a problem hiding this comment.
The new AtomTypeError fallback path for TS symmetry-number calculation introduces important behavior changes, but no corresponding test changes are shown. Add a unit test that triggers rmgpy.exceptions.AtomTypeError from the superclass symmetry calculation and asserts the fallback returns the expected symmetry number (and that it prefers the Species(...).get_symmetry_number() path when available).
| - `export AUTOTST="your_folder/AutoTST` | ||
|
|
||
| - `export PYTHONPATH=$AUTOTST:$PYTHONPATH` | ||
| - `export PYTHONPATH=/path/to/AutoTST:$PYTHONPATH` |
There was a problem hiding this comment.
The README update removes the AUTOTST environment variable guidance, but the repo still has many historical references to $AUTOTST in code paths (this PR replaces several, but not necessarily all). Clarify in the README whether AUTOTST is fully deprecated, and document the new path resolution strategy (repo-root discovery via autotst.utils.paths) so users know how to configure custom database/test locations.
calvinp0
force-pushed
the
autotst_update
branch
2 times, most recently
from
f8283eb to
a49a855
Compare
minor fix Improve TS symmetry number calculation error handling and add fallback tests Specifically catch RMG-related errors like ValueError and AtomTypeError during symmetry calculation for transition states. Added a logging warning when falling back to the molecule graph and implemented unit tests to verify fallback behavior. Skip symmetry number test for unoptimised transition states Isolate symmetry number checks into a separate test method and mark it as skipped. The current implementation for unoptimised transition states returns a symmetry number of 1 instead of the expected 3, as accurate calculation requires an optimised geometry.
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
…ance test paths Refactor job polling interval and improve scratch directory handling Add a configurable poll_interval parameter to the Job class to replace hardcoded wait times, facilitating faster test execution. Additionally, centralize scratch directory resolution into a helper method and improve the robustness of input file movement logic.
… and test data directories
…improve path handling minor fix minor fix
…pdate test setup to utilize utility functions for directory paths
Replace nosetests with pytest and simplify the cleanup logic for temporary test directories.
Configure a continuous integration pipeline to run pytest within a Conda environment on pushes, pull requests to the main branch, and a weekly schedule.
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).
This pull request modernizes and standardizes the test suite and path handling across the codebase, improves CI integration, and refactors code for better maintainability. The main themes are improved test reliability, removal of deprecated environment variable usage, and enhanced CI/CD automation.
Continuous Integration and Testing Improvements:
.github/workflows/ci.yml) to run automated tests on pushes, pull requests, and a weekly schedule, usingpytestand conda environments for consistency.Makefileto usepytestinstead ofnosetestsand improved test cleanup commands.Path Handling and Environment Variable Refactoring:
$AUTOTSTenvironment variable andos.path.expandvarswith utility functions likeproject_root(),test_dir(),test_data_dir(), anddatabase_dir()throughout the test suite (e.g., inautotst/calculator/gaussian_test.py,orca_test.py,statmech_test.py,vibrational_analysis_test.py,base_test.py,inputoutput_test.py). This makes tests more robust and less reliant on external environment configuration. [1] [2] [3] [4] [5] [6] [7]README.mdto clarify that onlyPYTHONPATHneeds to be set, removing the suggestion to setAUTOTST.Code Quality and Maintenance:
autotst/calculator/gaussian.pyto safely remove the"force"parameter from calculator objects using.pop("force", None)instead ofdel, preventing errors if the key is missing. [1] [2] [3] [4] [5] [6]These changes collectively improve the reliability, maintainability, and portability of the codebase, especially for testing and CI/CD workflows.