From 99f62d2fb87c63f781ac3e5c1afcbe97a8cd8c32 Mon Sep 17 00:00:00 2001
From: Eugene Park <jsukpark@users.noreply.github.com>
Date: Tue, 6 Aug 2024 09:36:35 -0500
Subject: [PATCH] Bugfix: `io.pwscf.PWInput.from_str()` (#3931)

* Bugfix: `io.pwscf.PWInput.from_str()`

Fixes wrong parsing of:
- Namelist options ending with comma (`,`)
- `ATOMIC_SPECIES` and `ATOMIC_POSITIONS` cards for structures with oxidation states

* Additional fix in regexp

- Must only have one of either + or - in oxidation states, not one of both

---------

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
---
 src/pymatgen/io/pwscf.py |  6 +++---
 tests/io/test_pwscf.py   | 26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/pymatgen/io/pwscf.py b/src/pymatgen/io/pwscf.py
index 73aa43f4a55..abcbf2bb8c2 100644
--- a/src/pymatgen/io/pwscf.py
+++ b/src/pymatgen/io/pwscf.py
@@ -292,7 +292,7 @@ def input_mode(line):
                 if match := re.match(r"(\w+)\(?(\d*?)\)?\s*=\s*(.*)", line):
                     key = match[1].strip()
                     key_ = match[2].strip()
-                    val = match[3].strip()
+                    val = match[3].strip().rstrip(",")
                     if key_ != "":
                         if sections[section].get(key) is None:
                             val_ = [0.0] * 20  # MAX NTYP DEFINITION
@@ -306,7 +306,7 @@ def input_mode(line):
                         sections[section][key] = PWInput.proc_val(key, val)
 
             elif mode[0] == "pseudo":
-                if match := re.match(r"(\w+)\s+(\d*.\d*)\s+(.*)", line):
+                if match := re.match(r"(\w+\d*[\+-]?)\s+(\d*.\d*)\s+(.*)", line):
                     pseudo[match[1].strip()] = match[3].strip()
 
             elif mode[0] == "kpoints":
@@ -318,7 +318,7 @@ def input_mode(line):
 
             elif mode[0] == "structure":
                 m_l = re.match(r"(-?\d+\.?\d*)\s+(-?\d+\.?\d*)\s+(-?\d+\.?\d*)", line)
-                m_p = re.match(r"(\w+)\s+(-?\d+\.\d*)\s+(-?\d+\.?\d*)\s+(-?\d+\.?\d*)", line)
+                m_p = re.match(r"(\w+\d*[\+-]?)\s+(-?\d+\.\d*)\s+(-?\d+\.?\d*)\s+(-?\d+\.?\d*)", line)
                 if m_l:
                     lattice += [
                         float(m_l[1]),
diff --git a/tests/io/test_pwscf.py b/tests/io/test_pwscf.py
index ea6dd7c3af0..76870670178 100644
--- a/tests/io/test_pwscf.py
+++ b/tests/io/test_pwscf.py
@@ -376,6 +376,32 @@ def test_read_str(self):
         assert_allclose(lattice, pw_in.structure.lattice.matrix)
         assert pw_in.sections["system"]["smearing"] == "cold"
 
+    def test_write_and_read_str(self):
+        struct = self.get_structure("Graphite")
+        struct.remove_oxidation_states()
+        pw = PWInput(
+            struct,
+            pseudo={"C": "C.pbe-n-kjpaw_psl.1.0.0.UPF"},
+            control={"calculation": "scf", "pseudo_dir": "./"},
+            system={"ecutwfc": 45},
+        )
+        pw_str = str(pw)
+        assert pw_str.strip() == str(PWInput.from_str(pw_str)).strip()
+
+    def test_write_and_read_str_with_oxidation(self):
+        struct = self.get_structure("Li2O")
+        pw = PWInput(
+            struct,
+            control={"calculation": "scf", "pseudo_dir": "./"},
+            pseudo={
+                "Li+": "Li.pbe-n-kjpaw_psl.0.1.UPF",
+                "O2-": "O.pbe-n-kjpaw_psl.0.1.UPF",
+            },
+            system={"ecutwfc": 50},
+        )
+        pw_str = str(pw)
+        assert pw_str.strip() == str(PWInput.from_str(pw_str)).strip()
+
 
 class TestPWOutput(PymatgenTest):
     def setUp(self):