Handle invalid selective dynamics info in POSCAR #3539

DanielYang59 · 2024-01-02T05:29:51Z

Summary

Major changes:

Issue warning in from_str method when invalid selective dynamics info would found in POSCAR, to resolve POSCAR is ignoring selective dynamics #3008.
Clean up INCAR tag checkings in check_params method.
Fix missing type checkings intest_check_params of tests/io/test_inputs.py.

Minor changes:

Issue warning if elements cannot be determined when reading POSCAR from_file
Minor format improvements

Analysis of "Invalid selective dynamics info in POSCAR"

In the second POSCAR provided in #3008:

Test POSCAR
   1.000000000
    20.0000000000000000    0.0000000000000000    0.0000000000000000
     0.0000000000000000   23.0000000000000000    0.0000000000000000
     0.0000000000000000    0.0000000000000000   15.0000000000000000
   C     H  
   2     1
Selective dynamics
Cartesian
     2.201545747940     2.214990562232     1.000000000000 C T  T  T  
     3.625859708182     2.193941473662     1.000000000000 C T  T  T  
     4.326973622066     3.379594386626     1.000000000000 H T  T  T

Which interprets to:

Test POSCAR
1.0
  20.0000000000000000    0.0000000000000000    0.0000000000000000
   0.0000000000000000   23.0000000000000000    0.0000000000000000
   0.0000000000000000    0.0000000000000000   15.0000000000000000
C H
2 1
Selective dynamics
direct
   0.1100772873970000    0.0963039374883478    0.0666666666666667 F T T C
   0.1812929854091000    0.0953887597244348    0.0666666666666667 F T T C
   0.2163486811033000    0.1469388863750435    0.0666666666666667 F T T H

Seems intended by pymatgen (anything other than "T" would be treated "F"):

pymatgen/pymatgen/io/vasp/inputs.py

Lines 430 to 431 in 8aa1cf8

    
           if has_selective_dynamics: 
        
               selective_dynamics.append([tok.upper()[0] == "T" for tok in tokens[3:6]])

I looked into the VASP source code poscar.F (version 5.4.4.pl2) line 385 to 392 (I cannot paste the code here for copyright reasons), which defines the parsing mechanism of POSCAR by VASP. And it seems VASP reads the 3-5 (0-indexed) elements of the position line as selective dynamics, which deems such POSCAR invalid.

`Selective dynamics` tag dropped when all DOFs relaxed

Regarding the 1st POSCAR in the original issue. There is:

pymatgen/pymatgen/io/vasp/inputs.py

Lines 121 to 124 in 8aa1cf8

    
           if selective_dynamics is not None: 
        
               selective_dynamics = np.array(selective_dynamics) 
        
               if not selective_dynamics.all(): 
        
                   site_properties["selective_dynamics"] = selective_dynamics

So selective dynamics array would only be kept when there exists at least one degree of freedom (DOF) frozen. And the user used an example POSCAR where all DOFs are relaxed.

Should we issue a warning to avoid confusion? I think it's unnecessary (but why would people relax all DOFs and toggle selective dynamics at the same time in the first place).

TODO list

Issue warning when invalid selective dynamics info detected in POSCAR
(Need suggestions on this) Confirm such POSCARs are truly invalid
Update unit tests for such invalid POSCARs

pymatgen/io/vasp/inputs.py

JaGeo · 2024-01-03T06:46:10Z

Meanwhile I noticed this warning here:

pymatgen/pymatgen/io/lobster/inputs.py

Lines 637 to 644 in 8aa1cf8

if "SHA256" in data or "COPYR" in data:

warnings.warn(

"These POTCARs are not compatible with "

"Lobster up to version 4.1.0."

"\n The keywords SHA256 and COPYR "

"cannot be handled by Lobster"

" \n and will lead to wrong results."

)

I had some experience with this issue, and was lucky enough to get direct input from Dr. Ryky Nelson @rnels12 (first author of the LOBSTER paper). When this imcompatibility issue happens, the charge spilling could jump from under 5% to as much as around 40%. Meanwhile the fix is pretty straightforward, by simply removing the lines in POTCAR with SHA256 and COPYR, which is suggested by Dr. Nelson himself. I have tested this the fix and could confirm this could significant reduce charge spilling to normal range.

Should I proceed and implement this fix directly in pymatgen (make a backup copy of POTCAR and modify the current POTCAR)? Meanwhile I think I might need to send Dr. Nelson another email to confirm the fix, because unfortunately my previous communication with Dr. Nelson is lost (my previous university set up an email purging policy for some reason).

We implemented this warning after talking to @rnels12 and @QuantumChemist . If this happens, the POTCARs are not read correctly and the results you get are wrong. Of course, you can implement a fix but I am not sure what you are intending to do: generating POTCARs without the line?

JaGeo · 2024-01-03T06:50:31Z

Btw, many of the pymatgen implementations have been used for the above LOBSTER paper. 🙃

JaGeo · 2024-01-03T06:57:16Z

Side note: we should test if the latest LOBSTER version fixes this. If so, I would not spend time on this fix.

JaGeo · 2024-01-03T07:17:59Z

@naik-aakash might also be able to give further input and could probably check if the latest LOBSTER fixes the issue.

BTW, I happend to find your LobsterPy package several weeks ago and it's awesome. I would see if I could make some contributions to that soon.

Btw, We work on a JOSS at the moment. (Basically now would be a perfect time for a contribution)

JaGeo · 2024-01-03T07:26:04Z

And, yes, the fix mentioned above is essentially the fix that we implemented manually.

naik-aakash · 2024-01-03T11:09:15Z

Hi @DanielYang59, I can test if the latest LOBSTER version fixes this issue and update you here asap

JaGeo · 2024-01-03T13:46:06Z

And, yes, the fix mentioned above is essentially the fix that we implemented manually.

@JaGeo Thanks for the input. If you have time you might open up some issues and I would see if I could solve any (as I'm not quite familiar with your package at this moment) so may not actually know where to get started. All the best with your paper then!

I don't have time to implement a fix. Sorry. Let's just see if the latest LOBSTER resolves it. Thanks in any case.

pymatgen/entries/compatibility.py

pymatgen/io/vasp/inputs.py

DanielYang59 · 2024-01-04T06:08:53Z

tests/io/vasp/test_inputs.py

        with pytest.warns(BadIncarWarning) as record:
            incar = Incar(
                {
                    "ADDGRID": True,
                    "ALGO": "Normal",
                    "AMIN": 0.01,
-                    "AMIX": 0.2,


Simplify the test INCAR a bit, keep only one entry of each data type.

DanielYang59 · 2024-01-04T06:12:44Z

tests/io/vasp/test_inputs.py

-                        False,
-                        "f",
-                    ],  # Should be a list of bools
-                    "M_CONSTR": [True, 1, "string"],  # Should be a list of real numbers


The type checkings for the values of "LATTICE_CONSTRAINTS" and "M_CONSTR" were not implemented, as there isn't definition for the type of list elements in incar_parameters.json (such as List[bool]).

I don't know that it's possible to do more careful type checking the way with the current framework (e.g., LATTICE_CONSTRAINTS : Sequence[bool, bool, bool]) but that's good to note.

DanielYang59 · 2024-01-04T06:19:45Z

tests/io/vasp/test_inputs.py

-        assert "PHON_TLIST: is_a_str is not a list" in record[4].message.args
+        assert record[0].message.args[0] == "Cannot find NBAND in the list of INCAR tags"
+        assert record[1].message.args[0] == "METAGGA: Cannot find SCAM in the list of values"
+        assert record[2].message.args[0] == "EDIFF: (5+1j) is not a float"


warning for "EDIFF": 5 + 1j, # value should be a float was missing

janosh · 2024-02-09T14:58:31Z

@esoteric-ephemera if you have time, could you take a quick look at why PotcarSingle test_is_valid is now failing?

    def test_is_valid(self):
>       assert self.psingle_Fe.is_valid
E       AssertionError: assert False
E        +  where False = PotcarSingle(symbol='Fe', functional='PBE', TITEL='PAW_PBE Fe 06Sep2000', VRHFIN='Fe:  d7 s1', n_valence_elec=8).is_valid

DanielYang59 · 2024-02-10T01:37:58Z

I noticed another test is failing too @esoteric-ephemera :

============================================================ FAILURES ============================================================= 
_________________________________________________ TestPotcarSingle.test_is_valid __________________________________________________ 

self = <tests.io.vasp.test_inputs.TestPotcarSingle testMethod=test_is_valid>

    def test_is_valid(self):
>       assert self.psingle_Fe.is_valid
E       AssertionError: assert False
E        +  where False = PotcarSingle(symbol='Fe', functional='PBE', TITEL='PAW_PBE Fe 06Sep2000', VRHFIN='Fe:  d7 s1', n_valence_elec=8).is_valid
E        +    where PotcarSingle(symbol='Fe', functional='PBE', TITEL='PAW_PBE Fe 06Sep2000', VRHFIN='Fe:  d7 s1', n_valence_elec=8) = <tests.io.vasp.test_inputs.TestPotcarSingle testMethod=test_is_valid>.psingle_Fe

tests\io\vasp\test_inputs.py:1165: AssertionError
___________________________________ TestPotcarSingle.test_multi_potcar_with_and_without_sha256 ____________________________________ 

self = <tests.io.vasp.test_inputs.TestPotcarSingle testMethod=test_multi_potcar_with_and_without_sha256>

    def test_multi_potcar_with_and_without_sha256(self):
        filename = f"{FAKE_POTCAR_DIR}/POT_GGA_PAW_PBE_54/POTCAR.Fe_O.gz"
        potcars = Potcar.from_file(filename)
        # Still need to test the if POTCAR can be read.
        # No longer testing for hashes
        for psingle in potcars:
            if psingle.hash_sha256_from_file:
                assert psingle.sha256_computed_file_hash == psingle.hash_sha256_from_file
            else:
>               assert psingle.is_valid
E               AssertionError: assert False
E                +  where False = PotcarSingle(symbol='Fe_pv', functional='PBE', TITEL='PAW_PBE Fe_pv 02Aug2007', VRHFIN='Fe:  3pd7s1', n_valence_elec=14).is_valid

tests\io\vasp\test_inputs.py:1218: AssertionError
------------------------------------------------------ Captured stderr call ------------------------------------------------------- 
D:\GitHub\pymatgen\pymatgen\io\vasp\inputs.py:1784: UnknownPotcarWarning: POTCAR data with symbol Fe_pv is not known to pymatgen. Your POTCAR may be corrupted or pymatgen's POTCAR database is incomplete.
  warnings.warn(
D:\GitHub\pymatgen\pymatgen\io\vasp\inputs.py:1784: UnknownPotcarWarning: POTCAR data with symbol O is not known to pymatgen. Your POTCAR may be corrupted or pymatgen's POTCAR database is incomplete.
  warnings.warn(

But I think we should not be cramming too many topics into a single PR?

…] | Any", variable has type "tuple[Any, ...]") [assignment]'"

DanielYang59 · 2024-02-10T06:10:54Z

pymatgen/io/vasp/inputs.py

@@ -321,7 +321,7 @@ def from_str(cls, data, default_names=None, read_velocities=True) -> Poscar:
            raise ValueError("Empty POSCAR")

        # Parse positions
-        lines = tuple(clean_lines(chunks[0].split("\n"), remove_empty_lines=False))
+        lines = list(clean_lines(chunks[0].split("\n"), remove_empty_lines=False))


Fixed the mypy type annotation error: pymatgen\io\vasp\inputs.py:481: error: Incompatible types in assignment (expression has type "list[str]", variable has type "tuple[Any, ...]") [assignment]

DanielYang59 · 2024-02-10T06:12:30Z

pymatgen/io/vasp/inputs.py

@@ -312,7 +312,7 @@ def from_str(cls, data, default_names=None, read_velocities=True) -> Poscar:
            Poscar object.
        """
        # "^\s*$" doesn't match lines with no whitespace
-        chunks = re.split(r"\n\s*\n", data.rstrip(), flags=re.MULTILINE)
+        chunks: list[str] = re.split(r"\n\s*\n", data.rstrip(), flags=re.MULTILINE)


Fixed another mypy error: pymatgen\io\vasp\inputs.py:481: error: Incompatible types in assignment (expression has type "list[str] | Any", variable has type "tuple[Any, ...]") [assignment]

pymatgen/io/vasp/inputs.py

DanielYang59 · 2024-02-10T09:06:05Z

There is one last mypy type annotation error (unrelated to this PR but still be good if we could fix it), from the last line of this code block:

pymatgen/pymatgen/io/vasp/inputs.py

Lines 434 to 472 in 12ab947

    
           if read_velocities: 
        
               # Parse the lattice velocities and current lattice, if present. 
        
               # The header line should contain "Lattice velocities and vectors" 
        
               # There is no space between the coordinates and this section, so 
        
               # it appears in the lines of the first chunk 
        
               lattice_velocities = [] 
        
               if len(lines) > ipos + n_sites + 1 and lines[ipos + n_sites + 1].lower().startswith("l"): 
        
                   for line in lines[ipos + n_sites + 3 : ipos + n_sites + 9]: 
        
                       lattice_velocities.append([float(tok) for tok in line.split()]) 
        
               # Parse velocities if any 
        
               velocities = [] 
        
               if len(chunks) > 1: 
        
                   for line in chunks[1].strip().split("\n"): 
        
                       velocities.append([float(tok) for tok in line.split()]) 
        
               # Parse the predictor-corrector data 
        
               predictor_corrector = [] 
        
               predictor_corrector_preamble = None 
        
               if len(chunks) > 2: 
        
                   lines = chunks[2].strip().split("\n") 
        
                   # There are 3 sets of 3xN Predictor corrector parameters 
        
                   # So can't be stored as a single set of "site_property" 
        
                   # First line in chunk is a key in CONTCAR 
        
                   # Second line is POTIM 
        
                   # Third line is the thermostat parameters 
        
                   predictor_corrector_preamble = f"{lines[0]}\n{lines[1]}\n{lines[2]}" 
        
                   # Rest is three sets of parameters, each set contains 
        
                   # x, y, z predictor-corrector parameters for every atom in order 
        
                   lines = lines[3:] 
        
                   for st in range(n_sites): 
        
                       d1 = [float(tok) for tok in lines[st].split()] 
        
                       d2 = [float(tok) for tok in lines[st + n_sites].split()] 
        
                       d3 = [float(tok) for tok in lines[st + 2 * n_sites].split()] 
        
                       predictor_corrector.append([d1, d2, d3]) 
        
           else: 
        
               velocities = predictor_corrector = predictor_corrector_preamble = lattice_velocities = None

I got:

pymatgen\io\vasp\inputs.py:468: error: Item "None" of "list[Any] | None" has no attribute "append"  [union-attr]
pymatgen\io\vasp\inputs.py:474: error: Item "None" of "list[Any] | None" has no attribute "append"  [union-attr]
pymatgen\io\vasp\inputs.py:496: error: Item "None" of "list[Any] | None" has no attribute "append"  [union-attr]

A simple example to reproduce this type annotation error:

met_condition = True

if met_condition:
    foo = []

    for i in range(3):
        foo.append(i)

else:
    foo = None

I got error: Incompatible types in assignment (expression has type "None", variable has type "list[int]") [assignment].

If I change it to:

from typing import Union


met_condition = True

if met_condition:
    foo: Union[list, None] = []

    for i in range(3):
        foo.append(i)

else:
    foo = None

then the error message would become: error: Item "None" of "list[Any] | None" has no attribute "append" [union-attr]

Do you have any suggestions? @janosh Thanks!

janosh · 2024-02-10T09:10:50Z

Do you have any suggestions? @janosh Thanks!

instead of setting to None, you could initialize those variables to empty lists. that way, there's no type union. you'd only have to change the Poscar.__init__ to accommodate:

- if velocities is not None:
+ if velocities:

and likewise for the other variables

DanielYang59 · 2024-02-10T11:58:44Z

Thanks a lot @janosh. Updated and confirmed to fix the mypy error.

esoteric-ephemera · 2024-02-12T17:07:44Z

@DanielYang59, see my comment above, these tests pass outside this PR. They're failing because of changes made by this PR that are easy to fix - opened a PR on your fork to fix it.

pymatgen/io/vasp/inputs.py

janosh

thanks a lot @DanielYang59! this is much appreciated. 👍

DanielYang59 · 2024-02-13T08:59:34Z

Thanks a lot for reviewing @janosh and everyone for giving me suggestions @esoteric-ephemera @JaGeo @rkingsbury

DanielYang59 changed the title ~~Fix missing selective dynamics when reading VASP POASCR from string~~ Fix missing selective dynamics when reading VASP POSCAR from string Jan 2, 2024

This comment was marked as duplicate.

Sign in to view

DanielYang59 marked this pull request as draft January 2, 2024 11:18

DanielYang59 changed the title ~~Fix missing selective dynamics when reading VASP POSCAR from string~~ [Need advice] Selective dynamics is dropped when all degrees of freedom of all atoms are allowed to relaxed Jan 2, 2024

DanielYang59 changed the title ~~[Need advice] Selective dynamics is dropped when all degrees of freedom of all atoms are allowed to relaxed~~ [Need advice] Selective dynamics is dropped when all DOFs of all atoms are allowed to relaxed Jan 2, 2024

DanielYang59 marked this pull request as ready for review January 2, 2024 11:46

This comment was marked as resolved.

Sign in to view

DanielYang59 changed the title ~~[Need advice] Selective dynamics is dropped when all DOFs of all atoms are allowed to relaxed~~ Selective dynamics is dropped when all DOFs of all atoms are allowed to relaxed Jan 2, 2024

DanielYang59 changed the title ~~Selective dynamics is dropped when all DOFs of all atoms are allowed to relaxed~~ Selective dynamics dropped when all DOFs are relaxed/ Handle illegal POSCAR Jan 2, 2024

DanielYang59 changed the title ~~Selective dynamics dropped when all DOFs are relaxed/ Handle illegal POSCAR~~ Selective dynamics dropped when all DOFs are relaxed/ Handle invalid POSCAR Jan 2, 2024

DanielYang59 commented Jan 3, 2024

View reviewed changes

pymatgen/io/vasp/inputs.py Show resolved Hide resolved

DanielYang59 commented Jan 3, 2024

View reviewed changes

pymatgen/io/vasp/inputs.py Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as off-topic.

Sign in to view

DanielYang59 commented Jan 4, 2024

View reviewed changes

pymatgen/entries/compatibility.py Show resolved Hide resolved

DanielYang59 commented Jan 4, 2024

View reviewed changes

pymatgen/io/vasp/inputs.py Show resolved Hide resolved

DanielYang59 commented Jan 4, 2024

View reviewed changes

Merge branch 'materialsproject:master' into fix-selective-dynamics

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

b01c4bb

break a super long line

Loading
Loading status checks…

ac3ee7f

DanielYang59 requested review from JaGeo, shyuep and mkhorton as code owners February 10, 2024 05:33

DanielYang59 added 2 commits February 10, 2024 13:43

fix type annotations errors related to this PR

Loading
Loading status checks…

0bbab02

fix " Incompatible types in assignment (expression has type "list[str…

Loading
Loading status checks…

7bae2ba

…] | Any", variable has type "tuple[Any, ...]") [assignment]'"

DanielYang59 commented Feb 10, 2024

View reviewed changes

pymatgen/io/vasp/inputs.py Show resolved Hide resolved

initialize vars with empty instead of None to fix type annotation error

Loading
Loading status checks…

88cbdfd

pre-commit auto-fixes

Loading
Loading status checks…

8b437af

DanielYang59 added 3 commits February 13, 2024 08:53

Merge branch 'master' into fix-selective-dynamics

Loading
Loading status checks…

249c264

revert var name from d to dct

Loading
Loading status checks…

5ded387

fix unit tests

Loading
Loading status checks…

e901682

DanielYang59 commented Feb 13, 2024

View reviewed changes

pymatgen/io/vasp/inputs.py Show resolved Hide resolved

simplify Lobsterin.get_basis()

Loading
Loading status checks…

cf4cecf

janosh approved these changes Feb 13, 2024

View reviewed changes

janosh enabled auto-merge (squash) February 13, 2024 08:06

janosh merged commit 00749d7 into materialsproject:master Feb 13, 2024
22 checks passed

DanielYang59 mentioned this pull request Feb 13, 2024

Improve INCAR tag check #3621

Merged

DanielYang59 deleted the fix-selective-dynamics branch February 23, 2024 02:26

DanielYang59 mentioned this pull request Mar 1, 2024

Remove BadPoscarWarning when POSCAR elements set by POTCAR #3662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle invalid selective dynamics info in POSCAR #3539

Handle invalid selective dynamics info in POSCAR #3539

DanielYang59 commented Jan 2, 2024 •

edited

Loading

This comment was marked as duplicate.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

JaGeo commented Jan 3, 2024

JaGeo commented Jan 3, 2024

JaGeo commented Jan 3, 2024

This comment was marked as resolved.

JaGeo commented Jan 3, 2024 •

edited

Loading

JaGeo commented Jan 3, 2024

naik-aakash commented Jan 3, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

JaGeo commented Jan 3, 2024

This comment was marked as off-topic.

DanielYang59 Jan 4, 2024

DanielYang59 Jan 4, 2024 •

edited

Loading

esoteric-ephemera Jan 30, 2024

DanielYang59 Jan 4, 2024

janosh commented Feb 9, 2024

DanielYang59 commented Feb 10, 2024 •

edited

Loading

DanielYang59 Feb 10, 2024

DanielYang59 Feb 10, 2024

DanielYang59 commented Feb 10, 2024

janosh commented Feb 10, 2024

DanielYang59 commented Feb 10, 2024

esoteric-ephemera commented Feb 12, 2024 •

edited

Loading

janosh left a comment

DanielYang59 commented Feb 13, 2024

	if has_selective_dynamics:
	selective_dynamics.append([tok.upper()[0] == "T" for tok in tokens[3:6]])

	if selective_dynamics is not None:
	selective_dynamics = np.array(selective_dynamics)
	if not selective_dynamics.all():
	site_properties["selective_dynamics"] = selective_dynamics

Handle invalid selective dynamics info in POSCAR #3539

Handle invalid selective dynamics info in POSCAR #3539

Conversation

DanielYang59 commented Jan 2, 2024 • edited Loading

Summary

Analysis of "Invalid selective dynamics info in POSCAR"

Selective dynamics tag dropped when all DOFs relaxed

TODO list

This comment was marked as duplicate.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

JaGeo commented Jan 3, 2024

JaGeo commented Jan 3, 2024

JaGeo commented Jan 3, 2024

This comment was marked as resolved.

JaGeo commented Jan 3, 2024 • edited Loading

JaGeo commented Jan 3, 2024

naik-aakash commented Jan 3, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

JaGeo commented Jan 3, 2024

This comment was marked as off-topic.

DanielYang59 Jan 4, 2024

Choose a reason for hiding this comment

DanielYang59 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

esoteric-ephemera Jan 30, 2024

Choose a reason for hiding this comment

DanielYang59 Jan 4, 2024

Choose a reason for hiding this comment

janosh commented Feb 9, 2024

DanielYang59 commented Feb 10, 2024 • edited Loading

DanielYang59 Feb 10, 2024

Choose a reason for hiding this comment

DanielYang59 Feb 10, 2024

Choose a reason for hiding this comment

DanielYang59 commented Feb 10, 2024

janosh commented Feb 10, 2024

DanielYang59 commented Feb 10, 2024

esoteric-ephemera commented Feb 12, 2024 • edited Loading

janosh left a comment

Choose a reason for hiding this comment

DanielYang59 commented Feb 13, 2024

DanielYang59 commented Jan 2, 2024 •

edited

Loading

`Selective dynamics` tag dropped when all DOFs relaxed

JaGeo commented Jan 3, 2024 •

edited

Loading

DanielYang59 Jan 4, 2024 •

edited

Loading

DanielYang59 commented Feb 10, 2024 •

edited

Loading

esoteric-ephemera commented Feb 12, 2024 •

edited

Loading