Nexus: update PWscf analyzer by jtkrogel · Pull Request #6196 · QMCPACK/qmcpack

jtkrogel · 2026-09-01T20:53:38Z

This PR generalizes and hardens the PWscf analyzer class. Capabilities are limited to scf, nscf, and relax runs.

Many real input/output files from PWscf are included. These are there to support testing here and for the roll-forward.

GPT-5.6 Sol was used

# Conflicts: # nexus/nexus/pwscf_analyzer.py

prckent · 2026-09-01T22:34:54Z

What are the plans to document the analyzer?

jtkrogel · 2026-09-02T00:32:24Z

I plan to make a "Working with PWscf" manual section with analyzer use as a subsection. I can add a first version after this PR if desired. I also have a few expanding PRs to add on to this (code is done, just rolling out a portion at a time to aid review).

Let me know your preference.

# Conflicts: # nexus/nexus/examples/qmcpack/rsqmc_misc/excited/band.py # nexus/nexus/examples/quantum_espresso/relax_Ge_T_vs_kpoints/relax_vs_kpoints_example.py # nexus/nexus/pwscf_analyzer.py

brockdyer03

Another partial review since I don't have enough time to comb through the LLM's code. As I expected, there are very similar problems in this PR as in #6194.

Things like using obj for class attributes inherently precludes automatic documentation and makes this code harder to maintain.

As a general remark, I want to stress that more flexible code does not always mean the code is more robust.

brockdyer03 · 2026-09-02T18:31:07Z

+# ``the spin up/dw Fermi energies are 5.1 5.2 ev``.  Missing eV units, three
+# energies, ``highest occupied level``, and prose merely mentioning Fermi
+# energy fail.
+fermi_energies_pattern = (


Only used in one place, move inline.

brockdyer03 · 2026-09-02T18:31:58Z

+        for line in lines:
+            if not has_cell and line.strip().startswith('CELL_PARAMETERS'):
+                has_cell = True
+            if not has_bfgs and 'BFGS Geometry Optimization' in line:


Use elif, avoids checking for each thing on lines that may have already had the preceding thing.

The independent flags are intentionally cumulative

That does not affect my comment. If a line has one of the strings then it does not have the other strings on it as well, so it should continue to the next line and not check the line for the other things.

brockdyer03 · 2026-09-02T18:46:27Z

+            self.read_energies(lines)
+            self.read_pressure(lines)
+            self.read_stress(lines)
+            self.read_forces(lines)


Why do we read forces for scf but not structure, when clearly the forces rely on being able to parse a structure?

The structure is not changing from the already known input structure

brockdyer03 · 2026-09-02T18:47:28Z

+
+    def read_band_edges(self):
+        """Add VBM and CBM energies to a parsed bands object."""
+        bands = self.bands


Why alias this? Just call self.bands

Local variable is clearer to read

brockdyer03 · 2026-09-02T18:49:04Z

+                unit.append(coordinates)
+            if not valid:
+                continue
+            self.kpoints_cart = np.array(cart,dtype=float)


Again, why are we storing these as their own objects instead of in a Structure? Then we don't need to store both cartesian and unit coordinate versions since Structure can do the conversion on its own.

Same reason as above

brockdyer03 · 2026-09-02T18:56:27Z

+                aforces = []
+                j       = i+1
+                while j<len(lines):
+                    match = re.search(atomic_force_pattern,lines[j])


Why are we using regex for something that can be done with a simple string split?

jtkrogel · 2026-09-02T21:34:30Z

Comments addressed. Final changes made.

brockdyer03

A lot of problems and changes.

I am unsure of this code in its entirety. There are so many small problems and minute details that it would take ages to find everything.

brockdyer03 · 2026-09-08T19:57:18Z

+                        float(value.lower().replace('d','e'))
+                        for value in values
+                        )
+        if len(fermi_energies)>0:


As far as I know, all successful QE calculations should have a Fermi energy printed. If this has zero, it should log an error/warning.

brockdyer03 · 2026-09-08T19:58:13Z

+        fermi_energies = []
+        for line in lines:
+            if 'Fermi energ' in line:
+                match = re.search(fermi_energies_pattern,line)


No need for a regex here. The lines are printed as

the Fermi energy is 10.1198 ev

A simple str.split() will work just fine.

brockdyer03 · 2026-09-09T01:46:08Z

+                if value is not None:
+                    energy = value
+        if energy is not None:
+            self.E = energy


The implicit else part of this branch makes this is a silent failure. Add logging of some sort.

brockdyer03 · 2026-09-09T01:49:33Z

-    return time
-#end def pwscf_time
+            calculation = 'scf'
+        self.calculation = calculation


It looks like this defaults to scf if a known mode is not found. If QE adds any more run modes (not sure what that would be) this will silently assume scf, which is bad practice.

brockdyer03 · 2026-09-09T01:52:48Z

+# Match the Fermi-energy result without collecting unrelated numbers earlier
+# on the line.  Real singular forms include ``the Fermi energy is 10.1198 ev``
+# and ``the Fermi energy = -3.22772442 eV``; spin-polarized output may report
+# ``the spin up/dw Fermi energies are 5.1 5.2 ev``.  Missing eV units, three


I see no example files with the following formatting:

the spin up/dw Fermi energies are 5.1 5.2 ev

Why are we accepting that?

brockdyer03 · 2026-09-09T01:56:49Z

+                    conf.axes = axes
+                    i+=3
+                else:
+                    conf = None


Silent failure.

brockdyer03 · 2026-09-09T01:59:32Z

+    )
+
+
+def parse_float(text):


This has the opportunity to fail silently every single time it is called, absolutely bad form. We should know if something is a float or not, and if this just fails silently then we are opening up the rest of the code to huge problems later on.

brockdyer03 · 2026-09-09T02:03:38Z

+            if len(aforces)>0 and (nat is None or len(aforces)==nat):
+                forces.append(aforces)
+        if len(forces)>0:
+            self.forces = np.array(forces,dtype=float)


Silent failure

brockdyer03 · 2026-09-09T02:04:09Z

+                    ]
+                unit.append(coordinates)
+            if not valid:
+                continue


Silent failure

brockdyer03 · 2026-09-09T02:18:32Z

There are a lot of problems I am noticing in this parser.

The most egregious is the countless situations where a silent failure can occur. I've marked some of the more obvious ones, but there are just so many it's hard to even find them all.

The next problem is the copious use of obj. There are many, many situations where a data structure is created with obj that requires documenting it elsewhere. The code should largely be self-documenting, and this certainly is not. I let the obj slide in the RMG analyzer because I was, at the time, unaware of how much of a burden it would be.

Another issue is that every read_ function sets the internal variable instead of returning it. This makes the code extremely difficult, if not impossible, to unit test. Ideally the read_ functions would be static methods unless it is absolutely required that they have access to self, which many of them do not need.

I haven't even gotten to PwscfAnalyzer yet, so after discussion of the comments on PwscfOutData, I will review that next.

The silent failures are not what you think. They are generally part of the design, i.e. read what is available and don't crash on parse. The physical quantities that are not available will return "None" when queried via the public/user-facing API. The exceptions raised on those relate to truly invalid requests on the part of the user; not failures in the code itself. The behavior of the read_* functions is intentional within this framework (parse/store/query).

I will remind you that obj is part of the Nexus codebase and in no way invalidates code committed within a PR.

jtkrogel added 23 commits August 27, 2026 16:47

nexus: first fix

d8f0e7b

nexus: harden pwscf analyzer

9c87ea4

nexus: bind to results data member

14284b3

nexus: convert to staticmethods

9858d1b

nexus: restructure

c358b49

nexus: consolidate logic and update usage

bdd5990

nexus: style updates

96d2383

nexus: refactor 1

b96a471

nexus: compaction, formatting, docstrings

8eacde9

nexus: more style

5cfa44d

Merge remote-tracking branch 'main/develop' into nx_pwscf_analyzer_dev

97e691f

# Conflicts: # nexus/nexus/pwscf_analyzer.py

nexus: further extend pwscf analyzer

698ae90

nexus: more quants and docs

0a334a9

nexus: expand xml data extraction

83f56ea

nexus: extend propery queries

32bc2a1

nexus: normalize docstrings

9da08a5

nexus: more docstring

7f8c050

nexus: add input/output tests

6ee0380

Merge remote-tracking branch 'main/develop' into nx_pwscf_analyzer_dev2

6da7f02

nexus: pass over unused

6dc0db6

nexus: style

99e6b96

nexus: ruff

0adbaf7

nexus: small updates

9cb8d74

github-actions Bot added nexus documentation python Pull requests that update python code labels Sep 1, 2026

prckent marked this pull request as draft September 1, 2026 21:12

jtkrogel added 3 commits September 1, 2026 17:13

nexus: cutback 1

a5276c7

nexus: cutback 2 and compatibility

c6e883a

nexus: formatting

4486a65

jtkrogel marked this pull request as ready for review September 1, 2026 21:33

jtkrogel mentioned this pull request Sep 2, 2026

Nexus: update RMG analyzer #6194

Merged

jtkrogel added 4 commits September 2, 2026 10:13

nexus: format

0a9b9f4

nexus: format

5246c5b

nexus: reduce regex

c71ba9c

nexus: merge line iterations

b02ed3f

rcclay self-requested a review September 2, 2026 16:06

rcclay previously approved these changes Sep 2, 2026

View reviewed changes

jtkrogel added 2 commits September 2, 2026 12:17

nexus: shorten

b60b88f

Merge remote-tracking branch 'main/develop' into nx_pwscf_analyzer_dev2

240d2a5

# Conflicts: # nexus/nexus/examples/qmcpack/rsqmc_misc/excited/band.py # nexus/nexus/examples/quantum_espresso/relax_Ge_T_vs_kpoints/relax_vs_kpoints_example.py # nexus/nexus/pwscf_analyzer.py

jtkrogel dismissed rcclay’s stale review via 240d2a5 September 2, 2026 17:00

rcclay previously approved these changes Sep 2, 2026

View reviewed changes

brockdyer03 requested changes Sep 2, 2026

View reviewed changes

nexus: address review comments

fa85333

jtkrogel dismissed rcclay’s stale review via fa85333 September 2, 2026 21:32

Merge remote-tracking branch 'main/develop' into nx_pwscf_analyzer_dev2

cf61bde

brockdyer03 requested changes Sep 9, 2026

View reviewed changes

Uh oh!

Conversation

jtkrogel commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prckent commented Sep 1, 2026

Uh oh!

jtkrogel commented Sep 2, 2026

Uh oh!

brockdyer03 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtkrogel commented Sep 2, 2026

Uh oh!

brockdyer03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtkrogel Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jtkrogel commented Sep 1, 2026 •

edited

Loading

jtkrogel Sep 9, 2026 •

edited

Loading