-
Notifications
You must be signed in to change notification settings - Fork 20
Major restructuring #12
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
Conversation
appears to fix NHC propagator
…ging - Introduced `AFSSHVVPropagator` and `AFSSHPropagator` for surface hopping dynamics. - Refactored `SurfaceHoppingMD` to utilize the new propagator. - Updated event logging to differentiate between hop and other events, including changes to YAML trace files. - Move `SHVVPropagator` and related code from `surface_hopping_md.py` to `surface_hopping_propagator.py`. - Adjusted trajectory YAML files to reflect new hop log structure.
- Updated the AdiabaticMD class to accept initial velocities instead of momenta. - Adjusted trajectory generation classes (TrajGenConst, TrajGenNormal, TrajGenBoltzmann) to use velocities. - Modified tests to reflect changes from momentum to velocity, ensuring consistency across examples and tests. - Cleaned up related calculations and variable names for clarity.
- Updated `TrajectoryCum` to serve as a factory for `SurfaceHoppingMD` with cumulative hopping enabled. - Enhanced `SurfaceHoppingMD` to support cumulative hopping, including initialization and probability management. - Adjusted data logging to include cumulative probability when applicable. - Cleaned up unused methods and improved code clarity.
- Refactored `TrajectoryCum` to initialize `SurfaceHoppingMD` with 'hopping_method' set to 'cumulative'. - Modified `SurfaceHoppingMD` to replace 'use_cumulative_hopping' with 'hopping_method', enforcing valid options and adjusting related logic. - Enhanced code clarity and consistency in handling hopping methods.
…ory class - Updated documentation to reflect changes in surface hopping methods, including the addition of `AugmentedFSSH` and `EvenSamplingTrajectory`. - Replaced instances of `TrajectoryCum` with `SurfaceHoppingMD` using the `hopping_method` parameter for cumulative hopping. - Removed the `TrajectoryCum` class to streamline the codebase and enhance clarity. - Adjusted related files to ensure consistency with the new implementation.
- Updated docstrings in `adiabatic_md.py`, `adiabatic_propagator.py`, `afssh.py`, `ehrenfest.py`, `even_sampling.py`, `propagator.py`, `surface_hopping_md.py`, `surface_hopping_propagator.py`, `tracer.py`, `typing.py`, and `models/electronics.py` to provide clearer descriptions of parameters, return values, and class functionalities. - Improved clarity and consistency in the documentation to facilitate better understanding of the codebase and its usage.
- Enhanced error messages for initial state and hopping method options to provide clearer feedback. - Removed the unused trouble_shooter method to streamline the code. - Updated exception handling to be more specific, improving code robustness and clarity.
- Added 'sphinx.ext.napoleon' extension to support Google-style docstrings in Sphinx documentation. - Updated autodoc options to include class and __init__ docstrings for better clarity. - Corrected model references in documentation files to use the proper module path `mudslide.models`. - Improved parameter documentation in `SurfaceHoppingMD` to enhance clarity and organization.
- Introduced a new example script `tully_avoided_crossing.py` to demonstrate the Tully avoided crossing model using surface hopping dynamics. - Added a test suite in `test_tully_avoided_crossing.py` to validate the functionality of the Tully avoided crossing implementation, including tests for cumulative and cumulative integrated hopping methods.
…ping_method parameters
- Removed unnecessary `__future__` imports and reorganized import statements for clarity. - Streamlined the code in `batch.py`, `even_sampling.py`, `periodic_table.py`, `propagator.py`, `surface.py`, `tracer.py`, and `turbo_make_harmonic.py` to enhance readability and maintainability. - Improved consistency in import styles and reduced clutter in the codebase.
…e abstract methods
…onian`, `force_matrix`, `derivative_coupling_tensor`.
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 refactors trajectory infrastructure to use velocities as the primary variable, introduces a Propagator abstraction for both adiabatic and surface-hopping MD, adds a Nose–Hoover thermostat, and reorganizes batch drivers and examples.
- Swapped “momentum” inputs and storage to “velocity” everywhere and updated generators and trajectories accordingly
- Introduced
Propagatorclasses (e.g.VVPropagator,NoseHooverChainPropagator,AFSSHVVPropagator) and factories (AdiabaticPropagator,AFSSHPropagator) - Restructured
BatchedTraj, renamedTrajectorySH→SurfaceHoppingMD, unified cumulative hopping option, and updated CLI and docs
Reviewed Changes
Copilot reviewed 110 out of 110 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| mudslide/batch.py | Switched to velocities in generators, restructured batch API |
| mudslide/main.py | Updated CLI to use SurfaceHoppingMD and velocity sampling |
| mudslide/afssh.py | Added Propagator classes and updated A-FSSH implementation |
| mudslide/adiabatic_propagator.py | New module for MD propagators including NHC thermostat |
Comments suppressed due to low confidence (2)
mudslide/afssh.py:75
- The return type annotation
'SHPropagator'is incorrect—update it toAFSSHVVPropagatoror a common basePropagator_to match actual return types.
def __new__(cls, model: Any, prop_options: Any = "vv") -> 'SHPropagator':
mudslide/main.py:141
- Assigning
trace_typeto a string will break theTraceManagerconstructor which expects a trace class. Restore the mapping to actual classes (e.g.{'memory': InMemoryTrace, 'yaml': YAMLTrace}[args.log]).
trace_type = args.log
| for x0, p0, initial, params in self.traj_gen(nsamples): | ||
| traj_input = self.options | ||
| for x0, v0, initial, params in self.traj_gen(nsamples): | ||
| traj_input = self.traj_options |
Copilot
AI
Jun 6, 2025
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.
Currently self.traj_options is reused and mutated in each loop iteration, leading to accumulated parameters across trajectories. Use a copy (e.g. traj_input = self.traj_options.copy()) before updating with params.
| traj_input = self.traj_options | |
| traj_input = self.traj_options.copy() |
| print("Nose-Hoover chain thermostat initialized with:") | ||
| print(f" Temperature: {temperature:.2f} K") | ||
| print(f" Number of chains: {nchains}") | ||
| print(f" Timescale: {timescale / fs_to_au:.2f} fs") | ||
| print(f" Thermostat mass: {self.nh_mass}") |
Copilot
AI
Jun 6, 2025
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.
[nitpick] Use the project logger instead of print for consistency and better control. For example: logger.info("Nose–Hoover chain thermostat initialized with:").
| print("Nose-Hoover chain thermostat initialized with:") | |
| print(f" Temperature: {temperature:.2f} K") | |
| print(f" Number of chains: {nchains}") | |
| print(f" Timescale: {timescale / fs_to_au:.2f} fs") | |
| print(f" Thermostat mass: {self.nh_mass}") | |
| logger.info("Nose-Hoover chain thermostat initialized with:") | |
| logger.info(f" Temperature: {temperature:.2f} K") | |
| logger.info(f" Number of chains: {nchains}") | |
| logger.info(f" Timescale: {timescale / fs_to_au:.2f} fs") | |
| logger.info(f" Thermostat mass: {self.nh_mass}") |
Lots of intertwined restructuring is done here:
AdiabaticMDTrajectorySH->SurfaceHoppingMDSurfaceHoppingMD, rather than a distinct classcumulative_integrated, following the way described in the Parker, Schiltz appendix.nparticles(e.g., number of atoms)ndims(i.e., 1D or 3D space)ndof(defaults tonparticlesxndims)dimensionality-> (nparticles,ndims)atom_types-> list of elements or what have youTrace_work well