Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some Kpoints generated using wrong mesh types #3245

Merged
merged 6 commits into from
Aug 18, 2023

Conversation

matthewkuner
Copy link
Contributor

Addresses #3220. Also adds a lot of tests to (hopefully) make sure nothing like this happens again.

@matthewkuner
Copy link
Contributor Author

matthewkuner commented Aug 17, 2023

@janosh seems like the tests that are failing are unrelated? Not 100% sure. If they are indeed unrelated, this should be ready for merging

…float]]

>       self._space_group_data = _get_symmetry_dataset(self._cell, symprec, angle_tolerance)
@janosh
Copy link
Member

janosh commented Aug 17, 2023

Tests were fine on master. Looks like something you changed turned magmons from tuple to list. I pushed a fix.

self._cell
>>> (((...), (...), (...)), ((...),), (1,), ([...],))

if len(magmoms) > 0:
self._cell: tuple[Any, ...] = (
tuple(map(tuple, structure.lattice.matrix.tolist())),
tuple(map(tuple, structure.frac_coords.tolist())),
tuple(zs),
tuple(map(tuple, magmoms)),
)
else: # if no magmoms given do not add to cell
self._cell = (
tuple(map(tuple, structure.lattice.matrix.tolist())),
tuple(map(tuple, structure.frac_coords.tolist())),
tuple(zs),
)
self._space_group_data = _get_symmetry_dataset(self._cell, symprec, angle_tolerance)

@matthewkuner
Copy link
Contributor Author

matthewkuner commented Aug 17, 2023

@janosh This could possibly be discovering some other issue with the magmoms, because I don't think a single line from my new code has anything to do with magmoms? ¯_(ツ)_/¯ unsure

         if len(magmoms) > 0:
            self._cell: tuple[Any, ...] = (
                tuple(map(tuple, structure.lattice.matrix.tolist())),
                tuple(map(tuple, structure.frac_coords.tolist())),
                tuple(zs),
>               tuple(map(tuple, magmoms)),
            )
@janosh
Copy link
Member

janosh commented Aug 17, 2023

Not sure either why this is surfacing now. I think this is good to go except for one thing. Do we need to add 3 new POSCAR files? How about you pick some of 31 that we already have:

from collections import defaultdict
from glob import glob

from pymatgen.io.vasp.inputs import Poscar
from pymatgen.symmetry.analyzer import SpacegroupAnalyzer
from pymatgen.util.testing import TEST_FILES_DIR

# glob all POSCARs in test files dir and count their crystal types
crys_sys_counts = defaultdict(int)
test_potcars = [*glob(f"{TEST_FILES_DIR}/*POSCAR*"), *glob(f"{TEST_FILES_DIR}/**/*POSCAR*")]
print(f"{len(test_potcars)=}")

for filepath in test_potcars:
    try:
        poscar = Poscar.from_file(filepath)
        crys_sys = SpacegroupAnalyzer(poscar.structure).get_crystal_system()
        crys_sys_counts[crys_sys] += 1
    except Exception:
        pass

crys_sys_counts = {'cubic': 15, 'trigonal': 3, 'hexagonal': 3, 'monoclinic': 3, 'orthorhombic': 2, 'triclinic': 2}

@janosh janosh changed the title [FIX] Fix major bug where some Kpoints are generated using wrong mesh types Fix some Kpoints generated using wrong mesh types Aug 17, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.57% ⚠️

Comparison is base (ccc1831) 74.65% compared to head (2bf1d96) 74.08%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3245      +/-   ##
==========================================
- Coverage   74.65%   74.08%   -0.57%     
==========================================
  Files         230      230              
  Lines       69384    69386       +2     
  Branches    16154    16155       +1     
==========================================
- Hits        51799    51408     -391     
- Misses      14513    14941     +428     
+ Partials     3072     3037      -35     
Files Changed Coverage Δ
pymatgen/analysis/chemenv/utils/scripts_utils.py 10.61% <0.00%> (ø)
pymatgen/symmetry/analyzer.py 87.89% <ø> (ø)
pymatgen/io/vasp/inputs.py 87.14% <100.00%> (+0.33%) ⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shyuep shyuep merged commit b33bced into materialsproject:master Aug 18, 2023
@matthewkuner matthewkuner deleted the fix_auto_kpoints branch October 12, 2023 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants