Issue 52 n reduced kpoints - #54
Merged
Merged
Conversation
n_reduced_kpoints fell back to nk1 * nk2 * nk3 when pymatgen was missing or SpacegroupAnalyzer raised AttributeError. Both are ordinary integers, so a caller could not tell a fallback from a real count -- while a cubic cell reduces by up to 48. The wrong value reached AiiDA extras and the published column, and goldilocks-core will port this module to size memory and to choose npool, where a silently wrong number is worse than a raised error. pymatgen was never really optional: build_gamma_kmesh_entries reads structure.lattice.reciprocal_lattice throughout, so a caller already needs a pymatgen Structure. Move it out of the kmesh extra and into dependencies. The AttributeError branch was load-bearing for the tests, which built fake Structure dataclasses and therefore asserted ladder behaviour while every n_reduced_kpoints was quietly the unreduced count. The fixtures now build real orthorhombic cells whose reciprocal lengths are exactly the values each test pins, so the assertions are unchanged and the reduction actually runs. Build one SpacegroupAnalyzer per ladder rather than one per rung; it depends on the structure alone. Closes #52
The fixtures were synthetic lattices carrying only reciprocal lengths, and they hid two things. Symmetry reduction never ran. SpacegroupAnalyzer raises AttributeError on a structure without sites, which the fallback swallowed, so every ladder assertion in the suite was made while n_reduced_kpoints was quietly the unreduced count. The repeated-mesh skip never fired either. The test named for it asserted that the ladder holds no duplicate, which is true whether or not the skip ran, and on an idealised lattice no duplicate is ever produced. The comment explained it by axes of equal |b_i| sharing change points -- but equal axes give identical quotients, which collapse in the candidate set before any mesh is computed. It takes axes that are almost equal: the two change points sit a hair apart and the sliver between them rounds onto its neighbour's mesh. Scanning the campaign's 20,826 MC3D cells finds 36 that do this and no idealised lattice that does. Fixtures are now diamond silicon, graphite and rutile, built from their space group and published lattice constants, plus MC3D 67775 read from the cell the campaign ran -- its a and b agree to about eleven decimal places, so it hits the skip. Every previous assertion is kept; the reduced counts and the skip are now pinned, and the docstring and reference page say what actually causes a repeat. Closes #52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.