Skip to content

Nexus: Rework physical_system.py#6010

Open
brockdyer03 wants to merge 26 commits into
QMCPACK:developfrom
brockdyer03:clean-physical-system
Open

Nexus: Rework physical_system.py#6010
brockdyer03 wants to merge 26 commits into
QMCPACK:developfrom
brockdyer03:clean-physical-system

Conversation

@brockdyer03

Copy link
Copy Markdown
Contributor

Proposed changes

This PR is a near-total rewrite of physical_system.py to remove the previous inheritance tree and replace with more specific classes.

Some notable changes are the removal of Matter, Particle, Particles, Ion, and PseudoIon. These have been replaced with Electrons, Positrons, and IonSpecies. The new classes offer a far less flexible API, which enhances discoverability and clarity.

Great care was taken to maintain the behavior of generate_physical_system, so in theory no user code should be broken by this change, however all I have to go on are the tests.

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

  • Refactoring
  • Testing changes
  • Documentation changes

Does this introduce a breaking change?

  • Maybe?

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.5
uv            0.11.24
cif2cell      2.1.0
coverage      7.13.5
h5py          3.16.0
matplotlib    3.10.9
numpy         2.4.4
pycifrw       4.4.6
pydot         4.0.1
pytest        9.0.3
pytest-cov    7.1.0
pytest-order  1.4.0
scipy         1.17.1
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)

…tronsBase`, update docstrings, add error tests
…s as keys, add hash function for making unordered sets
…init__()`, remove some unnecessary functions
@brockdyer03 brockdyer03 self-assigned this Jul 2, 2026
@brockdyer03 brockdyer03 added enhancement nexus refactor python Pull requests that update python code labels Jul 2, 2026
@brockdyer03 brockdyer03 mentioned this pull request Jul 6, 2026
37 tasks
@brockdyer03 brockdyer03 marked this pull request as ready for review July 6, 2026 18:49
@jtkrogel

jtkrogel commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

A few points upon a first skim:

  1. This PR is too large (+3008/-1181 lines), esp since the functionality it replaces is simple
  2. The large number of changes to the tests themselves makes it very hard to assess correctness
  3. The IonSpecies and Electrons classes are overly complex
  4. Possibly some of the changes to the PhysicalSystem API are unnecessary

I suggest we approach these changes in the following way:

  1. Make a first PR that only adds @property's to PhysicalSystem to hide the Particles, etc classes and simplify the interface in other parts of the code (e.g. QmcpackInput, etc). Update the tests to show equivalence to the original particles access patterns in other parts of the code.
  2. Make a second PR with just the Electrons/IonSpecies classes w/o replacing the existing Particles, etc classes. Introduce tests that demonstrate equivalence. Simplify these classes at that point.
  3. Make a third PR that replaces Particles, etc, within PhysicalSystem without otherwise changing the class. Update the PhysicalSystem tests in a very localized way, pinpointed on just those added classes.
  4. Have a discussion about what in PhysicalSystem itself needs changing. Make a PR as deemed appropriate.

@jtkrogel jtkrogel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above

@brockdyer03

Copy link
Copy Markdown
Contributor Author

I'll address these point by point.


  1. This PR is too large (+3008/-1181 lines), esp since the functionality it replaces is simple

The vast majority of this PR is in the testing module, which is +1,758/-379. There are another +428 lines of docstrings that I wrote for this, which accounts for 2186 lines.

The actual code changes are only ~900 lines, and a not-insignificant amount of that is whitespace changes.

This PR is not really a +3008/-1181, it's more like a +900/-900, when judging by actual code.


  1. The large number of changes to the tests themselves makes it very hard to assess correctness

The testing changes amount to the following (in test_physical_system.py):

  1. Remove system_same(), replace with explicit checks of properties. (-18 lines)
  2. Remove test_particle_initialization(), not needed since those classes don't exist. (-64 lines)
  3. Replace test for PhysicalSystem.kf_rpa() (+24/-14 lines)
  4. Split test_physical_system_initialization() into separate functions (+416/-282 lines)
  5. Replace test for PhysicalSystem.rename() (+73/-44 lines)
  6. Replace test for `PhysicalSystem.change_units() (+79/-26 lines)
  7. Replace test for PhysicalSystem.tile() (+59/-34 lines)
  8. Add a getter function for a reference LaAlO3 structure and tiled structure. (+113 lines)
  9. Add a getter function for a reference 2-atom diamond cell and 8-atom tiled version. (+106 lines)
  10. Add a test for the diamond reference (since it creates a Structure object, unlike LaAlO3) (+29 lines)
  11. Add test for Electrons class (+66 lines)
  12. Add test for Electrons.__eq__() (+24 lines)
  13. Add test for Electrons.__repr__() (+5 lines)
  14. Add test for Positrons class (+26 lines)
  15. Add test for Positrons.__repr__() (+5 lines)
  16. Add test for ensuring Electrons != Positrons (+30 lines)
  17. Add test for fully-specified IonSpecies instance (+23 lines)
  18. Add test for minimal IonSpecies instance (+17 lines)
  19. Add test for IonSpecies.__eq__() (+46 lines)
  20. Add test for IonSpecies.__repr__() (+5 lines)
  21. Add test for IonSpecies.__hash__() (+20 lines)
  22. Add test for IonSpecies.from_structure() (+40 lines)
  23. Add test for Electrons.neutralize_to() (+68 lines)
  24. Add test for PhysicalSystem.__init__() with a molecular system (+98 lines)
  25. Add test for PhysicalSystem.__init__() with a pre-tiled system (+90 lines)
  26. Add test for PhysicalSystem.__init__() with a file path instead of a structure (+31 lines)
  27. Add test for PhysicalSystem.group_atoms() (+77 lines)
  28. Add test for PhysicalSystem.ae_pp_species() (+19 lines)
  29. Add test for PhysicalSystem.large_Zeff_elem() (+19 lines)
  30. Add test for PhysicalSystem.copy() (+79 lines)
  31. Add miscellaneous tests for correct error raising (+48 lines)

To summarize, the only real changes to the testing are

  1. Explicit checks of system equality instead of system_same()
  • This accounts for a huge amount of line changes, since I now explicitly check the properties that were previously checked by system_same(), and by extension, structure_same().
  1. Remove test for non-existent classes
  2. Explicitly create the structure for test_kf_rpa(), instead of importing it from test_structure.py, making it more resilient to a refactor of those tests.
  3. Improve test_change_units() to actually check that all distances in the structure are changed, not just the last ones, and use new diamond reference instead of manually specified system
  4. Update test_rename() to reflect new classes
  5. Update test_tile() to explicitly check properties, instead of relying on structure_same(), and have it use the new LaAlO3 reference, which is a more complex example than the previous one, and thus means it is more robust.
  6. Split apart all of the various tests for generate_physical_system() into more appropriately named tests. I kept the actual calls to generate_physical_system() the same, so it is indeed using the same inputs. The largest changes there are because I got rid of system_same() in favor of explicit checks.
  • test_generate_struct()
  • test_generate_direct()
  • test_generate_with_path()
  • test_generate_gen()
  • test_generate_lookup()

  1. The IonSpecies and Electrons classes are overly complex

These classes provide an intuitive API, and several useful properties/functions so that developers and users don't need to custom-write their own versions. This makes them more resilient to a refactor since we can maintain the user interface while changing the backend.


  1. Possibly some of the changes to the PhysicalSystem API are unnecessary

The previous API was full of confusing behavior and functions that have been more appropriately relocated to other classes.

Some of the functions that were removed are PhysicalSystem.update_particles(), PhysicalSystem.update() and PhysicalSystem.add_particles(). By name, these felt like they did similar, if not the same exact thing, but PhysicalSystem.update_particles() essentially remade the entire physical system from scratch, while PhysicalSystem.update() only updated the net charge and net spin. PhysicalSystem.add_particles() appears to only have been called in PhysicalSystem.update_particles() and PhysicalSystem.generate_electrons(), and so could have trivially been replaced by in-place code.

Many functions were simply poorly constructed, e.g. PhysicalSystem.check_folded_system(). That function only checked if the structure of the folded system was the same as the system's structure's folded structure, but did not check that the particles were consistent. The new version checks the same thing as before, but now also checks the ions, electrons, and positrons. It also prints out a more well-formatted output in the event that they are different, which would help diagnose any issues.

There are more examples I could give, but the new version and the newly available functions are simply far more convenient and produce expected behavior, without the chance for behind the scenes changes to the system.


This PR absolutely should not be broken into 3 separate PRs. The total number of lines changed that are actual code changes is much smaller than the displayed +3008/-1181, and it is likely less than a thousand total. The changes to the test functions only make it more robust, and do not change what we were testing before.

The primary reason against splitting this PR is that it just leads to messy intermediate times while people could be developing against the new system, and are instead stuck with the old, undocumented, and nebulous version. In the end it just will take longer to tell if anything will actually break existing user code since we will have less time with people using the develop branch, and thus we are far more likely to let a breaking change into main. I could write tests for other modules that use PhysicalSystem and try to make extra sure that they don't break, but I have already spent hours poring over the existing code base to ensure I haven't missed anything, and I probably will spend even more hours doing that again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement nexus python Pull requests that update python code refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants