Create directories for model hamiltonians in tests directory#261
Create directories for model hamiltonians in tests directory#261JaeHyeokLee98 wants to merge 3 commits intoskmin-lab:masterfrom
Conversation
Create Shin-Metiu directory Move files related to Shin-Metiu modeln in tests to Shin-Metiu directory Create SAC directory
…-Metiu Copy mqc_runner.py, pytest.ini, test_mqc_cases.py
Summary of ChangesHello @JaeHyeokLee98, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a foundational testing framework for the Shin-Metiu model within the project. It introduces a dedicated runner for various multi-quantum coherence simulations and populates the test suite with extensive reference data. This setup is a prerequisite for expanding test coverage to other models, such as the SAC model, in future development. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a test runner and extensive reference data for the Shin-Metiu model, which is a great step towards improving test coverage. The overall structure is good. My review of the new test runner script, mqc_runner.py, focuses on several areas for improvement to enhance code quality, readability, and maintainability. I've suggested using Enums to avoid magic numbers, refactoring a long conditional block to reduce duplication, and avoiding modification of global state and the current working directory. I also pointed out a minor typo in the pytest configuration and some import styling that could follow PEP 8.
| def build_mqc(args: MQCArgs, nsteps: int = 10): | ||
|
|
||
| # Define initial conditions for the Shin-Metiu model | ||
| data["X1"] = 1836 # au |
There was a problem hiding this comment.
The function build_mqc modifies the global data dictionary imported from misc. Modifying global state can lead to unexpected side effects and makes the code harder to understand, debug, and test. It would be better to avoid this. Perhaps this value can be configured in a more explicit way, for example by passing it as an argument or loading it from a configuration file within the model that uses it.
| from dataclasses import dataclass | ||
| from contextlib import redirect_stdout, redirect_stderr | ||
| from pathlib import Path | ||
| import os, copy |
There was a problem hiding this comment.
According to PEP 8 style guidelines, it's recommended to have one import per line. This improves code readability.
| import os, copy | |
| import os | |
| import copy |
| import random | ||
|
|
||
| from molecule import Molecule | ||
| import qm, mqc |
There was a problem hiding this comment.
According to PEP 8 style guidelines, it's recommended to have one import per line. This improves code readability.
| import qm, mqc | |
| import qm | |
| import mqc |
| @dataclass(frozen=True) | ||
| class MQCArgs: | ||
| md: int = 3 # the MQC method (0: BOMD, 1: Eh, 2: SH, 3: SHXF, 4: EhXF, 5: CT) | ||
| rescale: int = 3 # the hop rescale option (0: energy, 1: velocity, 2: momentum, 3: augment) | ||
| reject: int = 1 # the hop reject option (0: keep, 1: reverse) | ||
| width: int = 0 # the width scheme in XF (0: frozen Gaussian as 0.1 Bohr, 1: TD) |
There was a problem hiding this comment.
The MQCArgs class uses integers to represent different configuration options (e.g., md, rescale). This use of 'magic numbers' can make the code harder to read and maintain. It's better to use Enums to define these options, which makes the code more self-documenting and robust.
Example:
from enum import IntEnum
class MQCMethod(IntEnum):
BOMD = 0
EH = 1
SH = 2
SHXF = 3
EHXF = 4
CT = 5
class RescaleOption(IntEnum):
ENERGY = 0
VELOCITY = 1
MOMENTUM = 2
AUGMENT = 3
# ... and so on for other options.
@dataclass(frozen=True)
class MQCArgs:
md: MQCMethod = MQCMethod.SHXF
rescale: RescaleOption = RescaleOption.AUGMENT
# ...This would make comparisons in build_mqc like if args.md == MQCMethod.BOMD: much clearer.
| if args.md == 0: # BOMD | ||
| out_dir += "-BOMD" | ||
| md = mqc.BOMD(molecule=mol, nsteps=nsteps, dt=5.0, unit_dt="au", istate=1) | ||
|
|
||
| elif args.md == 1: # Eh | ||
| out_dir += "-Eh" | ||
| md = mqc.Eh(molecule=mol, nsteps=nsteps, dt=5.0, unit_dt="au", istate=1) | ||
|
|
||
| elif args.md == 2: # SH | ||
| out_dir += "-SH" | ||
| md = mqc.SH(molecule=mol, nsteps=nsteps, nesteps=1, dt=5.0, unit_dt="au", istate=1) | ||
| suffix, md.hop_rescale, md.hop_reject = MOMENTUM_JUMP_SCHEME[(args.rescale, args.reject)] | ||
| out_dir += suffix | ||
|
|
||
| elif args.md == 3: # SHXF | ||
| out_dir += "-SHXF" | ||
| if args.width == 0: | ||
| md = mqc.SHXF(molecule=mol, nsteps=nsteps, nesteps=1, dt=5.0, unit_dt="au", sigma=0.1, istate=1) | ||
| out_dir += "-FG" | ||
| elif args.width == 1: | ||
| md = mqc.SHXF(molecule=mol, nsteps=nsteps, nesteps=1, dt=5.0, unit_dt="au", l_td_sigma=True, istate=1) | ||
| out_dir += "-TD" | ||
| else: | ||
| raise ValueError(f"Invalid width={args.width} for SHXF") | ||
| suffix, md.hop_rescale, md.hop_reject = MOMENTUM_JUMP_SCHEME[(args.rescale, args.reject)] | ||
| out_dir += suffix | ||
|
|
||
| elif args.md == 4: # EhXF | ||
| out_dir += "-EhXF" | ||
| if args.width == 0: | ||
| md = mqc.EhXF(molecule=mol, nsteps=nsteps, nesteps=1, dt=5.0, unit_dt="au", sigma=0.1, istate=1) | ||
| out_dir += "-FG" | ||
| elif args.width == 1: | ||
| md = mqc.EhXF(molecule=mol, nsteps=nsteps, nesteps=1, dt=5.0, unit_dt="au", l_td_sigma=True, istate=1) | ||
| out_dir += "-TD" | ||
| else: | ||
| raise ValueError(f"Invalid width={args.width} for EhXF") | ||
| suffix, md.hop_rescale, md.hop_reject = MOMENTUM_JUMP_SCHEME[(args.rescale, args.reject)] | ||
| out_dir += suffix | ||
|
|
||
| elif args.md == 5: # CT | ||
| out_dir += "-CT" | ||
| mol1 = copy.deepcopy(mol) | ||
| mol1.pos[0, 0] = -1.9 | ||
| md = mqc.CT(molecules=[mol, mol1], nsteps=nsteps, nesteps=1, dt=5.0, unit_dt="au", istates=[1, 1]) | ||
|
|
||
| else: | ||
| raise ValueError(f"Invalid md={args.md}") |
There was a problem hiding this comment.
The build_mqc function contains a long if/elif/else chain that can be refactored for better readability and maintainability. There is significant code duplication, especially in the blocks for SHXF (lines 58-69) and EhXF (lines 71-82). This duplication can be reduced by extracting common logic into helper functions or using a more data-driven approach like a factory pattern.
For instance, the logic for handling args.width and setting hop parameters is identical for SHXF and EhXF. This could be extracted into a shared function.
| cwd = os.getcwd() | ||
| try: | ||
| random.seed(1000) # keep random numbers for SH tests | ||
|
|
||
| with open(log_path, "w") as f, redirect_stdout(f), redirect_stderr(f): | ||
| md.run(qm=qm, output_dir=str(out_dir)) | ||
| #md.run(qm=qm, output_dir=out_dir) | ||
| finally: | ||
| os.chdir(cwd) |
There was a problem hiding this comment.
The run_case function changes the current working directory using os.chdir(). While the try...finally block correctly restores the original directory, changing the global state of the process (like the CWD) is generally discouraged. It can lead to subtle bugs, especially in a testing context where tests might be run in parallel. It's better to work with absolute paths or paths relative to a known base directory. The md.run function seems to accept an output_dir. If it can handle absolute paths, you could construct the absolute path for log_path and out_dir and pass those, avoiding the need to change the CWD.
| bomd: Born-Oppenheimer MD | ||
| eh: Ehrenfest dynamics | ||
| sh: surface hopping | ||
| shxf: surface hopping basd on XF |
There was a problem hiding this comment.
There is a typo in the description for the shxf marker. 'basd' should be 'based'.
shxf: surface hopping based on XF
| # Define initial conditions for the Shin-Metiu model | ||
| data["X1"] = 1836 # au | ||
| geom = """ |
There was a problem hiding this comment.
In the Tully's models, the mass was originally set to 2000 a.u., so please change it. Also, please edit all comments accordingly, e.g. Shin-Metiu -> SAC in this case.
| (3, 1): ("-a-", "augment", "reverse"), | ||
| } | ||
|
|
||
| def build_mqc(args: MQCArgs, nsteps: int = 50): |
There was a problem hiding this comment.
Consider reducing nsteps, let say, nsteps=10 by adjusting dt, etc., for faster pytest as in the Shin-Metiu pytest.
|
|
||
| return qm_model, md, out_dir | ||
|
|
||
| def run_case(args: MQCArgs, nsteps: int = 50): |
There was a problem hiding this comment.
Please make nsteps consistent when you change nsteps above.
2 participants
Previously, only Shin-Metiu model is tested with pytest.
I'm going to add SAC model also for pytest