A new script to generate a default aenet_input_dir from input.toml. - #86
skasamatsu wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A critical species-ordering issue and additional generation correctness issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a CLI script that generates default aenet training, prediction, fingerprint, and LAMMPS input files from input.toml.
Changes:
- Registers the
abics_gen_aenet_inputcommand. - Generates inputs based on configured species and solver type.
- Supports output-directory selection and forced overwrites.
File summaries
| File | Review |
|---|---|
pyproject.toml |
Registers the new CLI entry point. |
abics/scripts/gen_aenet_input.py |
Implements input generation. Species ordering must match the solver, all possible species must be included, multiple training directories must be handled, and forced regeneration must remove stale files. |
Review details
Suppressed comments (3)
abics/scripts/gen_aenet_input.py:60
- This derives the network species from one shuffled
config.structure, which only contains species currently selected by the configured counts. It omits valid species with zero initial occupancy (and any species that can later be introduced by a grand-canonical move), so the generated TYPES/NETWORKS and fingerprint files cannot represent all configurations from the input. Buildall_speciesfrom the base structure plus every non-vacancy group inconfig.defect_sublatticesbefore applyingignore_species.
all_species = set(config.structure.symbol_set)
ignore_species = set(params_root.get("train", {}).get("ignore_species", None) or [])
abics/scripts/gen_aenet_input.py:190
TrainerParamssupports multipletrain.base_input_direntries (andabics_trainrequires one generated input set per entry), but this silently selects only the first directory. With a multi-model/ensemble configuration, the command reports success while leaving the other trainer directories absent, so training later fails or uses incomplete inputs; either generate every configured directory or reject lists longer than one instead of discarding them.
base_input_dir = train_params.get("base_input_dir", "./aenet_train_input")
if isinstance(base_input_dir, list):
base_input_dir = base_input_dir[0]
output_dir = base_input_dir
abics/scripts/gen_aenet_input.py:202
--forceonly permits writing into non-empty directories; it does not remove files that are no longer part of the generated configuration. Re-running after changing the species set or switching solver modes therefore leaves stale fingerprint/network input files in the output tree, so the directory no longer represents the generated default set. Clear or replace each existing output directory whenforceis requested before regenerating it.
if os.path.exists(d) and os.listdir(d) and not force:
print(
f"Error: {d} already exists and is not empty. Use --force to overwrite.",
file=sys.stderr,
)
sys.exit(1)
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| species_list = " ".join(species) | ||
| lines = [ | ||
| f"pair_style aenet", | ||
| f"pair_coeff * * v00 {species_list} {HIDDEN_LAYERS_TAG}.nn {species_list}", |
There was a problem hiding this comment.
@copilot As written in one of the suppressed comments, the current implementation may not work for grand canonical setup. Please fix it. Also, regarding the assumption of alphabetical ordering, it should be safer for AenetPyLammpsSolver.calculate_energy to look at in.lammps for the generated ordering, I guess.
There was a problem hiding this comment.
Addressed in fed4c2d. The generator now derives species from the full configured species set (so grand-canonical cases are covered), aenetPyLammps uses sampling.solver.ignore_species for in.lammps, and AenetPyLammpsSolver now reads the atom-type ordering from in.lammps instead of inferring it from the structure.
There was a problem hiding this comment.
🟡 Changes recommended
Species ordering and ignore-species mismatches can produce incorrect or unusable LAMMPS mappings.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
abics/scripts/gen_aenet_input.py:153
- This still emits a sorted species list, but the consumer does not use that list to assign LAMMPS atom types:
AenetPyLammpsSolver.calculate_energyenumeratesst.symbol_setdirectly (aenet_pylammps.py:75-76), whileInput.from_directoryonly stores the input lines (aenet_pylammps.py:137-142). Since set iteration order need not match this alphabetical order, atom types can be mapped to the wrong aenet species and yield incorrect energies. Please make the solver parse the species order fromin.lammpsas requested in the previous review, or otherwise share one deterministic mapping between generator and solver.
f"pair_coeff * * v00 {species_list} {HIDDEN_LAYERS_TAG}.nn {species_list}",
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| species_list = " ".join(species) | ||
| lines = [ | ||
| f"pair_style aenet", | ||
| f"pair_coeff * * v00 {species_list} {HIDDEN_LAYERS_TAG}.nn {species_list}", |
Co-authored-by: skasamatsu <4555912+skasamatsu@users.noreply.github.com>
Co-authored-by: skasamatsu <4555912+skasamatsu@users.noreply.github.com>
Co-authored-by: skasamatsu <4555912+skasamatsu@users.noreply.github.com>
Co-authored-by: skasamatsu <4555912+skasamatsu@users.noreply.github.com>
Co-authored-by: skasamatsu <4555912+skasamatsu@users.noreply.github.com>
Co-authored-by: skasamatsu <4555912+skasamatsu@users.noreply.github.com>
Co-authored-by: skasamatsu <4555912+skasamatsu@users.noreply.github.com>
Co-authored-by: skasamatsu <4555912+skasamatsu@users.noreply.github.com>
Before, it was up to the user to provide a sensible aenet input file for training and prediction of the on-lattice model. This script automates that process.