Nexus: Move class variables to immutable types, remove constants created by `str.split()` by brockdyer03 · Pull Request #6058 · QMCPACK/qmcpack

brockdyer03 · 2026-07-27T19:07:14Z

Proposed changes

This PR takes several instances in Nexus of class variables, module variables, or constant collections and changing them from a mutable datatype to an immutable datatype. For example, many set-type variables were changed to frozenset, and many list variables were changed to tuple. This has the benefit of protection against accidental modification by a class method or user, and thus makes it less likely for errors to occur. In addition, in places where some constant variables are created using multiline-string splits, like the following:

machine_vars = set('''
    machine         account         machine_info    interactive_cores
    machine_mode    user
    '''.split())

these were moved to the appropriate collection type with constant string literals, e.g.

machine_vars = frozenset({
    'interactive_cores', 'machine_info', 'machine', 'machine_mode', 'user', 'account'
    })

This removes the runtime penalty incurred by splitting those strings, and can, in some cases, make test runs and initialization faster.

Additionally, a case was noticed in test_vasp_simulation.py::test_get_output_files where the test relied on order-specific lists, which was fixed to be order-agnostic.

What type(s) of changes does this code introduce?

Refactoring (no functional changes, no api changes)

Does this introduce a breaking change?

No

What systems has this change been tested on?

Laptop, Fedora Linux 43 (KDE Plasma Desktop Edition)
AMD Ryzen 7 PRO 7840U (8 cores, 16 logical processors)

Python        3.14.6
uv            0.11.26
cif2cell      2.1.0
coverage      7.15.0
h5py          3.16.0
matplotlib    3.11.0
numpy         2.5.1
pycifrw       4.4.6
pydot         4.0.1
pytest        9.1.1
pytest-cov    7.1.0
pytest-order  1.5.0
scipy         1.18.0
seekpath      2.2.1
spglib        2.7.0
sphinx        9.1.0

Checklist

- I have read the pull request guidance and develop docs
- This PR is up to date with the current state of 'develop'
- This PR adds tests to cover any new code, or to catch a bug that is being fixed
- Documentation has been added (if appropriate)

…ble types

…o immutable tuples and frozensets

jtkrogel

Changes for nested/repeated tuple reconstruction and discouraged style for unnecessarily set construction for short list queries of if statement literals.

… to tuples for immutability

jtkrogel

Marked all small set literals as resolved.

Some old comments outstanding; a couple now added.

jtkrogel · 2026-07-30T05:23:22Z


-    crystal_lmap = {0:'s',1:'sp',2:'p',3:'d',4:'f'}
-    crystal_lmap_reverse = dict(s=0,sp=1,p=2,d=3,f=4)
+    crystal_lmap = MappingProxyType({0:'s',1:'sp',2:'p',3:'d',4:'f'})


Is this really necessary? Please don't tell me you are going to decorate every dict in the codebase this way.

Only class variables. The MappingProxyType class just makes the dict immutable and changes nothing else about its interface. If you'd prefer, I can just add the ClassVar type annotation?

It's a roundabout frozendict?

jtkrogel · 2026-07-30T05:31:39Z

                setattr(cls,k,v)
        for v in ['attributes','elements','parameters','attribs','costs','h5tags']:
-            names = getattr(cls,v)
+            names = list(getattr(cls,v))


This just seems wrong. If sequence elements are intended to be mutated (as here), tuple is not the appropriate type (for attributes, elements, etc)

I thought so too initially, but the function there is basically only supposed to post-process the class attributes. Once the function is called, they should be "finalized" and not touched again.

If we really have to. It just unclear and unintuitive.

…3.15" This reverts commit 7cd021e.

jtkrogel

Good enough

prckent · 2026-07-31T16:03:43Z

Test this please

prckent · 2026-07-31T18:25:11Z

Test this please

prckent

Reapproving based on prior reviews

brockdyer03 added 3 commits July 27, 2026 10:55

Nexus: Change membership checks to use sets

994ab5b

Nexus: Change function argument defaults from mutable types to immuta…

54b7584

…ble types

Nexus: Convert mutable and str.split() class and module variables t…

9f7f5c9

…o immutable tuples and frozensets

brockdyer03 requested a review from jtkrogel July 27, 2026 19:07

brockdyer03 self-assigned this Jul 27, 2026

brockdyer03 added enhancement nexus cleanup For cleanling legacy lines of code refactor optimization python Pull requests that update python code labels Jul 27, 2026

Nexus: Catch tuples without trailing commas

3ad689f

jtkrogel requested changes Jul 28, 2026

View reviewed changes

Nexus: Address review comments

6020c0c

brockdyer03 requested a review from jtkrogel July 29, 2026 14:23

Nexus: Clear up more linter warnings, migrate QIxml-derived classes…

5204d6b

… to tuples for immutability

jtkrogel requested changes Jul 30, 2026

View reviewed changes

brockdyer03 added 3 commits July 30, 2026 07:49

Nexus: Use tuple addition instead of potentially more robust unpacking

a9dd430

Nexus: Alias MappingProxyType to frozendict until Python 3.15

7cd021e

Revert "Nexus: Alias MappingProxyType to frozendict until Python …

b365be5

…3.15" This reverts commit 7cd021e.

jtkrogel previously approved these changes Jul 30, 2026

View reviewed changes

Merge branch 'develop' into constant-frozenset

39e3059

brockdyer03 dismissed jtkrogel’s stale review via 39e3059 July 31, 2026 15:59

Nexus: Fix mistake from merge

74c8fa5

prckent enabled auto-merge July 31, 2026 16:03

prckent disabled auto-merge July 31, 2026 16:57

prckent mentioned this pull request Jul 31, 2026

Reorganize Ubuntu CI actions #6077

Merged

Merge branch 'develop' into constant-frozenset

e27c1b8

prckent approved these changes Jul 31, 2026

View reviewed changes

prckent merged commit 0990d7b into QMCPACK:develop Jul 31, 2026
50 checks passed

Uh oh!

Conversation

brockdyer03 commented Jul 27, 2026

Proposed changes

What type(s) of changes does this code introduce?

Does this introduce a breaking change?

What systems has this change been tested on?

Checklist

Uh oh!

jtkrogel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtkrogel left a comment

Choose a reason for hiding this comment

Uh oh!

jtkrogel Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

brockdyer03 Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

jtkrogel Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

jtkrogel Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

brockdyer03 Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

jtkrogel Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

jtkrogel left a comment

Choose a reason for hiding this comment

Uh oh!

prckent commented Jul 31, 2026

Uh oh!

prckent commented Jul 31, 2026

Uh oh!

prckent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants