Skip to content

[WIP] Add auto_ncl to VaspJob#369

Draft
mkhorton wants to merge 2 commits into
materialsproject:masterfrom
mkhorton:patch-2
Draft

[WIP] Add auto_ncl to VaspJob#369
mkhorton wants to merge 2 commits into
materialsproject:masterfrom
mkhorton:patch-2

Conversation

@mkhorton
Copy link
Copy Markdown
Member

@mkhorton mkhorton commented Apr 3, 2025

Allows Custodian to more easily work with non-collinear VASP.

Opening in draft, still WIP, needs tests.

mkhorton and others added 2 commits April 3, 2025 13:50
Allows Custodian to more easily work with non-collinear VASP.
Comment on lines +119 to +120
executable or with "gamma" replacing "std" in the name of
the VASP executable (typical setup in many systems). If so, run the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkhorton: I think this is very smart to do, seeing as VASP ships it this way (and not with .gamma). I don't see the change made to the source code though, so if that's the case, just a head's up!

to the last argument of the standard vasp_cmd.
ncl_vasp_cmd (str): Command for non-collinear vasp version when
auto_ncl is True. Should follow the list style of subprocess.
Defaults to none, which means "ncl" will replace "std" in
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VASP ships as vasp_ncl so that should probably be what is used.

@shyuep
Copy link
Copy Markdown
Member

shyuep commented May 12, 2026

Automated PR review generated by Claude (Opus 4.7) on behalf of @shyuep. Based purely on the diff and current GitHub Actions state — no code was executed locally.

Small, sensible addition that mirrors the existing auto_gamma pattern for non-collinear runs. CI (lint + all build matrix jobs) is green on the head commit. A few items before un-drafting:

  • ⚠️ Behavior change from the new default auto_ncl=True. Existing users with an LNONCOLLINEAR/LSORBIT INCAR and a vasp_ncl (or vasp_std.ncl) in PATH will silently switch executables on the next custodian run. For a 1.0-style stable utility, prefer auto_ncl=False and let users opt in, or at minimum call this out in the changelog.
  • ⚠️ cmd[-1].replace("std", "ncl") is brittle. If cmd[-1] doesn't contain "std" (e.g. vasp, vasp_gpu, or a path like /opt/vasp/std/vasp_std where "std" appears in a parent dir), the replace either no-ops (re-checks the same binary) or rewrites the path segment. Safer to gate on "std" in cmd[-1] before falling into that branch, or use a regex anchored to the basename.
  • ⚠️ Echoes Andrew's Aug 2025 comment: VASP's own build ships as vasp_ncl (analogous to vasp_std/vasp_gam), so the std→ncl swap is the realistic path for most users; the .ncl-suffix branch is mostly cosmetic. Consider documenting vasp_ncl as the canonical name.
  • ⚠️ Docstring drift for auto_gamma: the diff updates the auto_gamma docstring to advertise "gamma" replacing "std" behavior, but the surrounding run() block only implements the .gamma suffix — there is no std→gamma substitution. Either add that branch (parallel to the new auto_ncl logic) or revert the docstring change.
  • ℹ️ Tests: PR body already flags "needs tests." A unit test that monkeypatches shutil.which to assert each of the three resolution paths (ncl_vasp_cmd provided, .ncl suffix exists, std→ncl substitution exists, and the fall-through with no match) would be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants