ENH: Add an adaptive displacement list for atoms #209
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HaoZeke
force-pushed
the
addDispAtomScript
branch
from
607ba35 to
14291e1
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
Introduce a mechanism to run and cache per-state atom displacement scripts and wire it through the AKMC workflow, reducing redundant executions.
- Add a new
get_displacement_atom_listinStateto generate/cache atom lists via an external script - Introduce a new
displace_atom_kmc_state_scriptconfig field and propagate its use inakmcandexplorer - Implement a
_utilsmodule to securely run the external script and parse its output
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| meson.build | Register new _utils.py in install sources |
| eon/superbasinscheme.py | Refactor e_global_min calculation into two steps for clarity |
| eon/state.py | Added get_displacement_atom_list with caching to state metadata |
| eon/schema.py | Added displace_atom_kmc_state_script field to SaddleSearchConfig |
| eon/explorer.py | Load cached atom list and include it in generated job ini changes |
| eon/displace.py | (Accidental) debug print and new imports |
| eon/config.yaml | Document new script option under Saddle Search |
| eon/config.py | Parse new displace_atom_kmc_state_script from config file |
| eon/akmc.py | Invoke state-based script per AKMC state and assign its output |
| eon/_utils.py | New helper module to run and handle external atom list scripts |
Comments suppressed due to low confidence (2)
eon/_utils.py:1
- This new utility module contains nontrivial logic for running external scripts—consider adding unit tests to cover success, script-not-found, and error cases.
from pathlib import Path
eon/akmc.py:79
displace_atom_listis not defined in the config schema. This assignment won't be validated or propagated; consider adding it to the schema or using an existing field.
config.displace_atom_list = atom_list
| try: | ||
| # The list is stored as a string, so we need to parse it. | ||
| atom_list_str = self.info.get("Saddle Search", "displace_atom_list") | ||
| return atom_list_str |
There was a problem hiding this comment.
Method is annotated to return List[int] but returns a raw string. Consider parsing the comma-separated string into a list of ints or updating the return type.
Suggested change
| return atom_list_str | |
| return list(map(int, atom_list_str.split(','))) if atom_list_str else [] |
| # Merge potential files into invariants | ||
| invariants = dict(invariants, **io.load_potfiles(self.config.path_pot)) | ||
|
|
||
| atom_list_str = str(self.state.info.get("Saddle Search", "displace_atom_list", "")) |
There was a problem hiding this comment.
Directly reading the raw setting here bypasses the new get_displacement_atom_list logic. Consider calling that method to ensure generation and proper parsing.
Suggested change
| atom_list_str = str(self.state.info.get("Saddle Search", "displace_atom_list", "")) | |
| atom_list_str = str(self.get_displacement_atom_list()) |
eon/displace.py
Outdated
| #raise DisplaceError("Listed atoms are all frozen") | ||
| raise DisplaceError("Listed atoms are all frozen") | ||
|
|
||
| print(self.listed_atoms) |
There was a problem hiding this comment.
This appears to be a leftover debug print. Replace with logger.debug or remove to avoid cluttering standard output.
Suggested change
| print(self.listed_atoms) | |
| logger.debug(f"Listed atoms: {self.listed_atoms}") |
Comment on lines
+6
to
+9
| import sys | ||
| import subprocess | ||
| import tempfile | ||
| from pathlib import Path |
There was a problem hiding this comment.
These imports (sys, subprocess, tempfile, Path) look unused in this module. Consider removing them to keep dependencies clean.
Suggested change
| import sys | |
| import subprocess | |
| import tempfile | |
| from pathlib import Path |
i.e.
~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/users/home/rog32/Git/Github/QueensU/cuTvaccancy/.pixi/envs/default/lib/python3.13/site-packages/eon/akmc.py", line 296, in kmc_step
superbasining.register_transition(current_state, next_state)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/users/home/rog32/Git/Github/QueensU/cuTvaccancy/.pixi/envs/default/lib/python3.13/site-packages/eon/superbasinscheme.py", line 262, in register_transition
e_global_min = min ( e_min , ( state.get_energy() for state in start_statelist ) )
TypeError: '<' not supported between instances of 'generator' and 'float'
HaoZeke
force-pushed
the
addDispAtomScript
branch
from
aefabb8 to
bda0f92
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #208. Basically this also technically allows stuff like APE.
cc @iz1ng (might be of interest, server side change).
This isn't currently very efficient since it runs the script twice when it should only run it once (per new set of jobs), but that can be fixed in post.Now the list of atoms is part of the metadata for the state, so it is a bit more efficient, though each client search no longer gets a dynamic update, just the standard displacement one.