-
Notifications
You must be signed in to change notification settings - Fork 924
Incar.proc_val reuse incar_parameters.json to determine types
#4522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Incar.proc_val reuse incar_parameters.json to determine types
#4522
Conversation
tests/io/vasp/test_inputs.py
Outdated
| assert Incar.proc_val("LREAL", "on") == "On" | ||
|
|
||
| # TODO: need discussion (LORBIT should be int) | ||
| assert Incar.proc_val("LORBIT", "F") == "F" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to our recording:
pymatgen/src/pymatgen/io/vasp/incar_parameters.json
Lines 646 to 657 in bd29947
| "LORBIT": { | |
| "type": "int", | |
| "values": [ | |
| 0, | |
| 1, | |
| 2, | |
| 5, | |
| 10, | |
| 11, | |
| 12 | |
| ] | |
| }, |
LORBIT should be int , however one test file has it as bool:
<i type="logical" name="LORBIT"> F </i>
https://www.vasp.at/wiki/LORBIT
LORBIT = 0 | 1 | 2 | 5 | 10 | 11 | 12
Not sure if this is valid? Previous it's not in known int_keys so would not raise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to say if this was a permitted value in much older versions of VASP but I would favor only supporting the types VASP currently expects (int here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice thanks for the input! I would keep the recording as is then (also the proc_val should be more permissive so it would fallback to treat it as a string)
| return val.strip() | ||
|
|
||
| except ValueError: | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess proc_val should be more permissive (more strict check should be done by check_params)? i.e. if a tag's val is incompatible with its type in recording, I guess we just continue to try other conversations (and at the worst keep it as string) instead of error out?
cc @shyuep
febe86b to
2306a62
Compare
https://www.vasp.at/wiki/NUPDOWN NUPDOWN = [positive real] Default: NUPDOWN = not set Description: Sets the difference between the number of electrons in the up and down spin components. Allows calculations for a specific spin multiplet, i.e. the difference of the number of electrons in the up and down spin component will be kept fixed to the specified value. There is a word of caution required: If NUPDOWN is set in the INCAR file the initial moment for the charge density should be the same. Otherwise convergence can slow down. When starting from atomic charge densities (ICHARG=2), VASP will try to do this automatically by setting MAGMOM to NUPDOWN/NIONS. The user can of course overwrite this default by specifying a different MAGMOM (which should still result in the correct total moment). If one initializes the charge density from the one-electron wavefunctions, the initial moment is always correct, because VASP "pushes" the required number of electrons from the down to the up component. Initializing the charge density from the CHGCAR file (ICHARG=1), however, the initial moment is usually incorrect! If no value is set (or NUPDOWN=-1) a full relaxation will be performed. This is also the default.
2306a62 to
bbfc6f6
Compare
bbfc6f6 to
e21421e
Compare
Incar.proc_valreuseincar_parameters.jsonto determine types