Skip to content

Pass through None dataset in spglib dot access wrapper - #118

Merged
t-reents merged 2 commits into
materialscloud-org:mainfrom
ajjackson:fix-symmetrydetectionerror
Jan 29, 2026
Merged

Pass through None dataset in spglib dot access wrapper#118
t-reents merged 2 commits into
materialscloud-org:mainfrom
ajjackson:fix-symmetrydetectionerror

Conversation

@ajjackson

@ajjackson ajjackson commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

I have been trying to update the requirements of Euphonic to a recent seekpath and found that some combinations of seekpath and spglib were giving different error messages than expected when the symmetry cannot be detected. Specifically the behaviour is encountered when using seekpath 2.2 with older versions of spglib.

get_dot_access_dataset is a wrapper designed to convert old-style (dictionary access) spglib results to new-style (class attribute access) objects. Currently it does not consider the case that it is passed "None", which is the error signal for spglib<2.7 so if the wrapped spglib call returns None it can raise something like

../../.tox/py310-minrequirements-mac/lib/python3.10/site-packages/seekpath/getpaths.py:181: in get_path
    res = hpkot.get_path(
../../.tox/py310-minrequirements-mac/lib/python3.10/site-packages/seekpath/hpkot/__init__.py:162: in get_path
    dataset = get_dot_access_dataset(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
dataset = None

    def get_dot_access_dataset(dataset):
        """Return dataset with dot access.
    
        From spglib 2.5, dataset is returned as dataclass.
        To emulate it for older versions, this function is used.
    
        """
        spg_version = Version(version('spglib'))
    
        if spg_version < Version('2.5.0'):
            from types import SimpleNamespace
    
>           return SimpleNamespace(**dataset)
E           TypeError: types.SimpleNamespace() argument after ** must be a mapping, not NoneType

../../.tox/py310-minrequirements-mac/lib/python3.10/site-packages/seekpath/hpkot/tools.py:266: TypeError

I think this means the "if dataset is None" code block in seekpath.hpkot.__init__ has become unreachable. To have this work as expected, this PR allows None results to pass through the wrapper unchanged.

Here is a uv script (i.e. Python with dependencies declared in header) that reproduces the behaviour. You can get different results by changing the seekpath and spglib versions!

# /// script
# requires-python = "==3.10"
# dependencies = [
#     "packaging",
#     "seekpath[bz]==2.2.0",
#     "spglib==2.1",
# ]
# ///
import seekpath

cell = ([[2.426175880956926, -4.202259893915576, 0.0],
         [2.426175880956926, 4.202259893915576, 0.0],
         [0.0, 0.0, 5.35030451418839]],
        [],
        [])

output = seekpath.get_path(cell)

if output is None:
    print("Output is None")
else:
    print(f"Output: {output}")

@ajjackson
Pass through None dataset in spglib dot access wrapper
1062bcf 
get_dot_access_dataset is a wrapper designed to convert
old-style (dictionary access) spglib results to new-style (class
attribute access) objects. Currently it does not consider the case
that it is passed "None", which is the error signal for spglib<2.7.

This means that "if dataset is None" code block in
seekpath.hpkot.__init__ is probably unreachable. To have this work as
expected, allow None results to pass through the wrapper unchanged.
@ajjackson ajjackson mentioned this pull request Jan 27, 2026
Merged
@t-reents

t-reents commented Jan 29, 2026

Copy link
Copy Markdown
Collaborator

Thank you so much @ajjackson !

I was surprised that this is the case, but I just checked and this was actually integrated 1.5 years ago, after the previous 2.1 release. I initially assumed that this was already part of previous releases, and therefore working, when doing the refactoring for the 2.2 release.

Thanks again for spotting and fixing the issue!

@t-reents
t-reents merged commit 2ac6385 into materialscloud-org:main Jan 29, 2026
7 checks passed
shyuep added a commit to materialsproject/pymatgen-core that referenced this pull request Sep 16, 2026
@Yi-111-a @shyuep
tests: probe seekpath usability instead of gating on platform/Python …
00f5e60 
…version (#138)

* tests: probe seekpath usability instead of gating on platform/Python version

The seekpath-backed k-path tests were skipped on Windows and Python >= 3.13 by a
hard-coded platform/version check. Combined with the CI matrix that meant they
never ran on any CI job: the windows 3.13 leg installs no 'optional' extra so
importorskip fires, while the macos 3.13 and ubuntu 3.14 legs that do install
seekpath were excluded by the version branch.

The gate is stale. It was added in 3643cdb (2026-05-08) to work around a real
failure, but 9f92544 removed the 'optional' extra from the Windows leg 11 days
later, turning the platform branch into dead code while the version branch kept
suppressing coverage. The underlying bug was upstream: seekpath's
get_dot_access_dataset() did not handle the None dataset that spglib < 2.7
returns, fixed in materialscloud-org/seekpath#118 (released in 2.2.1), which
uv.lock already pins.

Replace the gate with an end-to-end probe over all seven crystal systems the
tests exercise, calling get_kpoints() so the probe covers the same code path.
The tests now run wherever seekpath works and skip with an accurate reason
wherever it does not.

Verified on Windows: py3.13 and py3.14 with seekpath 2.2.1 / spglib 2.7.0 now
run 11 tests (previously 5 skipped), the full 230-spacegroup sweep passes, and
py3.13 without the optional extra still skips cleanly. macOS/Linux not available
locally to test.

* tests: restore real modules after simulating seekpath absence

test_err_msg_on_seekpath_not_installed reloads pymatgen.symmetry.kpath
while seekpath is shadowed out of sys.modules. The reload executes the
module's import fallback, leaving `get_path = None` in its namespace, and
nothing restores it when the patch exits: sys.modules is rolled back, but
the module namespace stays poisoned for the rest of the session.

Any test that constructs a KPathSeek afterwards dies with
"'NoneType' object is not callable" at kpath.py:941. Under xdist this only
hits workers that also ran the pwmat tests, which made the failure look
intermittent - the same symptom that motivated the hard-coded
platform/Python skip in the seekpath tests (3643cdb).

Reload both modules again after the patch exits, in a finally so that a
failing assertion inside the block cannot skip the restore. This is also
what makes the probe-based gate from the previous commit viable: the
pollution happens at run time, after probes are evaluated at collection
time, so a probe alone cannot catch it.

* Address review on the seekpath test gate

1. The probe no longer swallows every exception. It used to catch bare
   Exception and return False, which converted any future regression in
   KPathSeek into a silent module skip -- the same failure mode this PR
   removes, one level down. It now tolerates only the two documented
   unavailability signatures (seekpath not installed, and spglib returning
   no dataset so kpath.get_path was left as None) and re-raises everything
   else. The caught exception text goes into the skip reason.

2. The duplicated probe is gone. seekpath_unusable_reason() now lives once
   in pymatgen.util.testing and is imported by both test modules, with
   functools.cache so the seven KPathSeek constructions happen once per
   session rather than once per importing module.

3. Kept the module reloads, deliberately -- see below.

On (3): switching to monkeypatch.setattr(kpath, "get_path", None) does not
work here. KPathSeek.__init__ is guarded by @requires(get_path is not None,
...) and monty's requires() captures that condition as a bool at decoration
time. Once kpath has been imported with seekpath importable the guard is
permanently satisfied, so blanking get_path afterwards only changes what
kpath.py:941 calls:

    >>> kpath.get_path = None
    >>> KPathSeek(struct).get_kpoints()
    TypeError: 'NoneType' object is not callable

whereas the test asserts RuntimeError "SeeK-path needs to be installed".
Re-executing the module is what re-evaluates the guard, so the reload is
load-bearing. Verified against pymatgen-core 2026.8.30 / seekpath 2.2.1 /
spglib 2.7.0.

The stale-class-object concern is real but does not bite here: importlib.reload
re-executes into the same module namespace, so a class object captured before
the reload still resolves get_path through kpath.__dict__ and heals once the
restore runs. To make that contract explicit rather than implicit, the test now
asserts kpath.get_path is not None at the end.

Verified locally on Windows, Python 3.13, pymatgen-core 2026.8.30,
seekpath 2.2.1, spglib 2.7.0, single process:

  tests/io/pwmat/test_inputs.py + tests/symmetry/test_kpath_hin.py
  + tests/symmetry/test_kpaths.py                      26 passed

  same trio with the `finally` restore removed          6 failed
  (the new assertion, plus the same 5 seekpath tests that failed on CI)

ruff check and ruff format clean on all four files.

* Address review on the seekpath test gate

1. The probe no longer swallows every exception. It used to catch bare
   Exception and return False, which converted any future regression in
   KPathSeek into a silent module skip -- the same failure mode this PR
   removes, one level down. It now tolerates only the two documented
   unavailability signatures (seekpath not installed, and spglib returning
   no dataset so kpath.get_path was left as None) and re-raises everything
   else. The caught exception text goes into the skip reason.

2. The duplicated probe is gone. seekpath_unusable_reason() now lives once
   in pymatgen.util.testing and is imported by both test modules, with
   functools.cache so the seven KPathSeek constructions happen once per
   session rather than once per importing module.

3. Kept the module reloads, deliberately -- see below.

On (3): switching to monkeypatch.setattr(kpath, "get_path", None) does not
work here. KPathSeek.__init__ is guarded by @requires(get_path is not None,
...) and monty's requires() captures that condition as a bool at decoration
time. Once kpath has been imported with seekpath importable the guard is
permanently satisfied, so blanking get_path afterwards only changes what
kpath.py:941 calls:

    >>> kpath.get_path = None
    >>> KPathSeek(struct).get_kpoints()
    TypeError: 'NoneType' object is not callable

whereas the test asserts RuntimeError "SeeK-path needs to be installed".
Re-executing the module is what re-evaluates the guard, so the reload is
load-bearing. Verified against pymatgen-core 2026.8.30 / seekpath 2.2.1 /
spglib 2.7.0.

The stale-class-object concern is real but does not bite here: importlib.reload
re-executes into the same module namespace, so a class object captured before
the reload still resolves get_path through kpath.__dict__ and heals once the
restore runs. To make that contract explicit rather than implicit, the test now
asserts kpath.get_path is not None at the end.

Verified locally on Windows, Python 3.13, pymatgen-core 2026.8.30,
seekpath 2.2.1, spglib 2.7.0, single process:

  tests/io/pwmat/test_inputs.py + tests/symmetry/test_kpath_hin.py
  + tests/symmetry/test_kpaths.py                      26 passed

  same trio with the `finally` restore removed          6 failed
  (the new assertion, plus the same 5 seekpath tests that failed on CI)

ruff check and ruff format clean on all four files.

* Address review on the seekpath test gate

1. The probe no longer swallows every exception. It used to catch bare
   Exception and return False, which converted any future regression in
   KPathSeek into a silent module skip -- the same failure mode this PR
   removes, one level down. It now tolerates only the two documented
   unavailability signatures (seekpath not installed, and spglib returning
   no dataset so kpath.get_path was left as None) and re-raises everything
   else. The caught exception text goes into the skip reason.

2. The duplicated probe is gone. seekpath_unusable_reason() now lives once
   in pymatgen.util.testing and is imported by both test modules, with
   functools.cache so the seven KPathSeek constructions happen once per
   session rather than once per importing module.

3. Kept the module reloads, deliberately -- see below.

On (3): switching to monkeypatch.setattr(kpath, "get_path", None) does not
work here. KPathSeek.__init__ is guarded by @requires(get_path is not None,
...) and monty's requires() captures that condition as a bool at decoration
time. Once kpath has been imported with seekpath importable the guard is
permanently satisfied, so blanking get_path afterwards only changes what
kpath.py:941 calls:

    >>> kpath.get_path = None
    >>> KPathSeek(struct).get_kpoints()
    TypeError: 'NoneType' object is not callable

whereas the test asserts RuntimeError "SeeK-path needs to be installed".
Re-executing the module is what re-evaluates the guard, so the reload is
load-bearing. Verified against pymatgen-core 2026.8.30 / seekpath 2.2.1 /
spglib 2.7.0.

The stale-class-object concern is real but does not bite here: importlib.reload
re-executes into the same module namespace, so a class object captured before
the reload still resolves get_path through kpath.__dict__ and heals once the
restore runs. To make that contract explicit rather than implicit, the test now
asserts kpath.get_path is not None at the end.

Verified locally on Windows, Python 3.13, pymatgen-core 2026.8.30,
seekpath 2.2.1, spglib 2.7.0, single process:

  tests/io/pwmat/test_inputs.py + tests/symmetry/test_kpath_hin.py
  + tests/symmetry/test_kpaths.py                      26 passed

  same trio with the `finally` restore removed          6 failed
  (the new assertion, plus the same 5 seekpath tests that failed on CI)

ruff check and ruff format clean on all four files.

* Address review on the seekpath test gate

1. The probe no longer swallows every exception. It used to catch bare
   Exception and return False, which converted any future regression in
   KPathSeek into a silent module skip -- the same failure mode this PR
   removes, one level down. It now tolerates only the two documented
   unavailability signatures (seekpath not installed, and spglib returning
   no dataset so kpath.get_path was left as None) and re-raises everything
   else. The caught exception text goes into the skip reason.

2. The duplicated probe is gone. seekpath_unusable_reason() now lives once
   in pymatgen.util.testing and is imported by both test modules, with
   functools.cache so the seven KPathSeek constructions happen once per
   session rather than once per importing module.

3. Kept the module reloads, deliberately -- see below.

On (3): switching to monkeypatch.setattr(kpath, "get_path", None) does not
work here. KPathSeek.__init__ is guarded by @requires(get_path is not None,
...) and monty's requires() captures that condition as a bool at decoration
time. Once kpath has been imported with seekpath importable the guard is
permanently satisfied, so blanking get_path afterwards only changes what
kpath.py:941 calls:

    >>> kpath.get_path = None
    >>> KPathSeek(struct).get_kpoints()
    TypeError: 'NoneType' object is not callable

whereas the test asserts RuntimeError "SeeK-path needs to be installed".
Re-executing the module is what re-evaluates the guard, so the reload is
load-bearing. Verified against pymatgen-core 2026.8.30 / seekpath 2.2.1 /
spglib 2.7.0.

The stale-class-object concern is real but does not bite here: importlib.reload
re-executes into the same module namespace, so a class object captured before
the reload still resolves get_path through kpath.__dict__ and heals once the
restore runs. To make that contract explicit rather than implicit, the test now
asserts kpath.get_path is not None at the end.

Verified locally on Windows, Python 3.13, pymatgen-core 2026.8.30,
seekpath 2.2.1, spglib 2.7.0, single process:

  tests/io/pwmat/test_inputs.py + tests/symmetry/test_kpath_hin.py
  + tests/symmetry/test_kpaths.py                      26 passed

  same trio with the `finally` restore removed          6 failed
  (the new assertion, plus the same 5 seekpath tests that failed on CI)

ruff check and ruff format clean on all four files.

* Fix the restore assertion and a probe comment label

Per re-review:

1. The restore check asserted kpath.get_path is not None, which reads as
   "seekpath is installed" when what it means is "the reload was undone".
   On the two legs that do not install the optional extra (no seekpath at
   all) get_path is legitimately None after a correct restore, and the
   assertion would have turned a passing test into a failure. It now
   captures get_path before the reload and asserts identity afterwards:
   None is None on no-seekpath legs, a real check everywhere else.

2. The trigonal entry in the probe lattice tuple is Lattice.hexagonal(2, 95)
   built with hexagonal axes, so the "# rhombohedral" label was wrong; it
   now reads "# trigonal (hexagonal axes)". The lattice itself was correct.

Verified locally on Windows, Python 3.13, pymatgen-core 2026.8.30,
seekpath 2.2.1, spglib 2.7.0:

  tests/io/pwmat/test_inputs.py + tests/symmetry/test_kpath_hin.py
  + tests/symmetry/test_kpaths.py                      26 passed

ruff check and ruff format clean on both touched files.

* Fix the restore assertion and a probe comment label

Per re-review:

1. The restore check asserted kpath.get_path is not None, which reads as
   "seekpath is installed" when what it means is "the reload was undone".
   On the two legs that do not install the optional extra (no seekpath at
   all) get_path is legitimately None after a correct restore, and the
   assertion would have turned a passing test into a failure. It now
   captures get_path before the reload and asserts identity afterwards:
   None is None on no-seekpath legs, a real check everywhere else.

2. The trigonal entry in the probe lattice tuple is Lattice.hexagonal(2, 95)
   built with hexagonal axes, so the "# rhombohedral" label was wrong; it
   now reads "# trigonal (hexagonal axes)". The lattice itself was correct.

Verified locally on Windows, Python 3.13, pymatgen-core 2026.8.30,
seekpath 2.2.1, spglib 2.7.0:

  tests/io/pwmat/test_inputs.py + tests/symmetry/test_kpath_hin.py
  + tests/symmetry/test_kpaths.py                      26 passed

ruff check and ruff format clean on both touched files.

* tests: probe seekpath with a single cubic cell

Per review: the seven-lattice loop always resolved P1 (general-position
sites), so it paid 7x for 1x coverage. Drop to one cubic cell and document
that the probe only checks KPathSeek is callable; note why the cache is
populated at collection time.

* tests: align seekpath probe with review (in-cell coords, get_path check)

- Fix probe fractional coords to in-cell values with to_unit_cell=True
- Detect unavailability via kpath.get_path is None instead of message text
- Drop @functools.cache (finally-restore removes the collection-order need)
- Condense reload comments in test_err_msg_on_seekpath_not_installed

* tests: evaluate the seekpath probe lazily via a module-scoped fixture

seekpath_unusable_reason() was called at module scope in test_kpaths.py and
test_kpath_hin.py, so a probe failure outside the known unavailability
paths aborted collection of the whole file — including tests that never
touch seekpath. A module-scoped `seekpath_usable` fixture in
tests/symmetry/conftest.py keeps the re-raise intent but confines the
error to tests that need seekpath. Also assert that the pwmat.inputs
reload in test_err_msg_on_seekpath_not_installed rebinds KPathSeek to the
restored module's class, closing the loop on both reloads.

---------

Co-authored-by: Shyue Ping Ong <shyuep@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t-reents t-reents t-reents approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ajjackson @t-reents