DPGEN with constrained structures (new) - #892
Conversation
To generate the constrained surface models proposed at discussion [deepmodeling#877](deepmodeling#877) Co-Authored-By: Cyanbeacon <86344464+Cyanbeacon@users.noreply.github.com>
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Thank you very much for the nice contribution. Could you please:
- explanation how to use the atom constrain in the document
- an illustrative example would be welcome
- add UTs that make sure the code is working in an expected way.
There was a problem hiding this comment.
Verdict: Changes requested. The constrained-layer implementation can select or skip the wrong atomic layers and can fail for valid parameter combinations, potentially producing scientifically incorrect constraints. The PR also lacks documentation, examples, and unit tests and conflicts with master. I recommend reimplementing it against the current surf.py.
Note: The Codex quota is about to reset, so I am using the remaining tokens to review all open PRs in this repository.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| if from_poscar: | ||
| shutil.copy2( pos_in, pos_out) | ||
| shutil.copy2( pos_in, pos_out) | ||
| if "fix_layers" and "total_layers" in jdata: |
There was a problem hiding this comment.
[Blocking] if "fix_layers" and "total_layers" in jdata checks only total_layers, because the non-empty string is always truthy. Providing only total_layers enters layer() and then fails because fix_layers is absent; providing only fix_layers never writes Selective dynamics. The layer step also needs to apply outside the from_poscar branch. Otherwise generated surfaces have no flags while poscar_shuffle() reads coordinates as if a Selective line were present.
| dis_small = cord_sort[0][2] | ||
| dis_big = cord_sort[-1][2] | ||
| max_dis = dis_big - dis_small | ||
| lay_space = max_dis / jdata["total_layers"] |
There was a problem hiding this comment.
[Blocking] max_dis / total_layers is off by one. For three layers at z=0,1,2 it computes a spacing of 2/3, so asking to fix the first two layers fixes only the first. At minimum, ideal equally spaced layers require total_layers - 1; a more robust implementation should cluster actual z coordinates with a tolerance to support relaxed or non-uniform surfaces. Add tests for one/two/multiple fixed layers, non-uniform layer spacing, and invalid parameter lengths.
3 participants
To generate the constrained surface models proposed at discussion #877
Co-Authored-By: Cyanbeacon 86344464+Cyanbeacon@users.noreply.github.com