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

Install optional boltztrap2 and pygraphviz in CI #3786

Merged
merged 58 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from 54 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
3b1b077
enable `boltztrap2`
DanielYang59 Apr 26, 2024
6568e05
Revert "enable `boltztrap2`"
DanielYang59 Apr 26, 2024
ec9b65e
use explicit encoding
DanielYang59 Apr 26, 2024
186ead5
install graphviz
DanielYang59 Apr 26, 2024
45e0dc7
increment boltztrap version to 24.1.1
DanielYang59 Apr 26, 2024
11d6a7d
fix bader patch
DanielYang59 Apr 26, 2024
108b440
pre-commit auto-fixes
pre-commit-ci[bot] Apr 26, 2024
eecb0fb
ruff fix
DanielYang59 Apr 26, 2024
5ce39ff
Merge branch 'install-opt-dep' of github.com:DanielYang59/pymatgen in…
DanielYang59 Apr 26, 2024
3f37027
tweak bader test
DanielYang59 Apr 26, 2024
78885a8
Merge branch 'master' into install-opt-dep
DanielYang59 Apr 28, 2024
c48f159
need confirm: add `wheel` to dev dep
DanielYang59 Apr 28, 2024
114c582
try install BoltzTraP2 with pip
DanielYang59 Apr 28, 2024
2c7cd55
add comment
DanielYang59 Apr 28, 2024
0386dac
try move bolt after numpy to fix win, remove wheel
DanielYang59 Apr 28, 2024
68b678a
try move boltrap2 to ubuntu only
DanielYang59 Apr 28, 2024
95a1ef9
more description more accurate
DanielYang59 Apr 28, 2024
78830a9
Merge branch 'master' into install-opt-dep
DanielYang59 Apr 29, 2024
04c038b
Merge branch 'master' into install-opt-dep
DanielYang59 May 2, 2024
57a95f2
Merge branch 'master' into install-opt-dep
DanielYang59 May 31, 2024
bf081ee
Merge branch 'master' into install-opt-dep
shyuep May 31, 2024
666f343
Merge branch 'master' into install-opt-dep
DanielYang59 Jun 1, 2024
d2cf959
Merge branch 'master' into install-opt-dep
DanielYang59 Jun 9, 2024
8e6e336
Merge branch 'master' into install-opt-dep
DanielYang59 Jul 13, 2024
3e6518b
install BoltzTraP2 in CI
DanielYang59 Jul 13, 2024
abca862
try to install BoltzTraP2 with uv
DanielYang59 Jul 13, 2024
780bb24
remove repeated numpy install in CI tests
DanielYang59 Jul 13, 2024
4128284
update np.exceptions.RankWarning
DanielYang59 Jul 13, 2024
d534473
Revert "update np.exceptions.RankWarning"
DanielYang59 Jul 13, 2024
771c311
Merge branch 'master' into install-opt-dep
DanielYang59 Jul 15, 2024
293c87e
install setuptool with uv
DanielYang59 Jul 15, 2024
f7859ef
Merge branch 'install-opt-dep' of https://github.com/DanielYang59/pym…
DanielYang59 Jul 15, 2024
c52f3c2
update BoltzTraP2
DanielYang59 Jul 15, 2024
0312792
Merge branch 'master' into install-opt-dep
DanielYang59 Jul 15, 2024
209a643
install wheel for building boltztrap2
DanielYang59 Jul 15, 2024
534c7a7
try to install boltztrap2 with pip not uv
DanielYang59 Jul 15, 2024
0c13672
Merge branch 'master' into install-opt-dep
DanielYang59 Jul 24, 2024
df545b4
Merge branch 'master' into install-opt-dep
DanielYang59 Aug 3, 2024
8a58a54
Merge branch 'master' into install-opt-dep
DanielYang59 Aug 4, 2024
9cc05d5
Merge branch 'master' into install-opt-dep
DanielYang59 Aug 4, 2024
5c279ea
Merge branch 'master' into install-opt-dep
DanielYang59 Aug 5, 2024
8d44806
install cython before boltztrap2
DanielYang59 Aug 5, 2024
d7cf794
try to install boltztrap2 with optional
DanielYang59 Aug 5, 2024
7849ad0
try to manually install bt2 in workflow
DanielYang59 Aug 5, 2024
1a8ba84
manually install bt2 with pip
DanielYang59 Aug 5, 2024
f036a6f
remove bt2 ver
DanielYang59 Aug 5, 2024
7f6bfb0
install bt2 only on ubuntu
DanielYang59 Aug 5, 2024
d15c58c
skip bt2 for windows
DanielYang59 Aug 5, 2024
1706c82
try to fix failing bt2 test
DanielYang59 Aug 5, 2024
3a3cd77
more descriptive skip msg
DanielYang59 Aug 5, 2024
bbdf250
finish typing of bzt2
DanielYang59 Aug 5, 2024
b81a163
try to install pygraphviz
DanielYang59 Aug 5, 2024
c67abbc
revert update to | replace for graphs
DanielYang59 Aug 5, 2024
155cbff
test.yml join install pymatgen and install dependencies steps
janosh Aug 5, 2024
57d34e2
fix TestStructureGraph.test_draw unhashable type list error
janosh Aug 5, 2024
6dfec4c
Merge branch 'master' into install-opt-dep
DanielYang59 Aug 5, 2024
5e242eb
skip BoltzTraP2 and matplotlib 3.9.1
DanielYang59 Aug 5, 2024
29032df
Merge branch 'master' into install-opt-dep
DanielYang59 Aug 6, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ jobs:
resolution: highest
extras: ci,optional
- os: ubuntu-latest
python: '>3.9'
python: ">3.9"
resolution: lowest-direct
extras: ci,optional
- os: macos-latest
python: '3.10'
python: "3.10"
resolution: lowest-direct
extras: ci # test with only required dependencies installed

Expand Down Expand Up @@ -70,18 +70,29 @@ jobs:
- name: Install ubuntu-only conda dependencies
if: matrix.config.os == 'ubuntu-latest'
run: |
micromamba install -n pmg -c conda-forge enumlib packmol bader openbabel openff-toolkit --yes
micromamba install -n pmg -c conda-forge enumlib packmol bader openbabel openff-toolkit pygraphviz --yes

- name: Install pymatgen and dependencies
run: |
micromamba activate pmg

# TODO remove temporary fix. added since uv install torch is flaky.
# track https://github.com/astral-sh/uv/issues/1921 for resolution
pip install torch --upgrade

uv pip install numpy cython
uv pip install cython setuptools wheel

uv pip install --editable '.[${{ matrix.config.extras }}]' --resolution=${{ matrix.config.resolution }}
DanielYang59 marked this conversation as resolved.
Show resolved Hide resolved

- name: Install optional Ubuntu dependencies
if: matrix.config.os == 'ubuntu-latest'
run: |
micromamba activate pmg

# TODO: uv cannot install BoltzTraP2 (#3786),
# suggesting no NumPy when there is
pip install BoltzTraP2

- name: pytest split ${{ matrix.split }}
run: |
micromamba activate pmg
Expand Down
5 changes: 1 addition & 4 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,7 @@ ci = ["pytest-cov>=4", "pytest-split>=0.8", "pytest>=8"]
docs = ["sphinx", "sphinx_rtd_theme"]
optional = [
"ase>=3.23.0",
# TODO restore BoltzTraP2 when install fixed, hopefully following merge of
# https://gitlab.com/sousaw/BoltzTraP2/-/merge_requests/18
# caused CI failure due to ModuleNotFoundError: No module named 'packaging'
# "BoltzTraP2>=22.3.2; platform_system!='Windows'",
"BoltzTraP2>=24.7.2 ; platform_system != 'Windows'",
"chemview>=0.6",
"chgnet>=0.3.8",
"f90nml>=1.1.2",
Expand Down
4 changes: 2 additions & 2 deletions src/pymatgen/analysis/graphs.py
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,7 @@ def draw_graph_to_file(
d["label"] = f"{d['weight']:.2f} {units}"

# update edge with our new style attributes
g.edges[u, v, k] |= d
g.edges[u, v, k].update(d)
Copy link
Contributor Author

@DanielYang59 DanielYang59 Aug 6, 2024

Choose a reason for hiding this comment

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

@janosh Forgot to mention this, I have to revert your update -> |= change here c67abbc because it seems to break unit test.

I haven't got a chance to look closer yet, perhaps the operator is not well defined for edges

Copy link
Member

Choose a reason for hiding this comment

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

yeah i noticed that, that was sloppy refactoring on my part. thanks for fixing!

perhaps the operator is not well defined for edges

that's very likely correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was sloppy refactoring on my part.

pygraphviz was not installed in CI at that time, so there's no way for you to catch that :)

Copy link
Member

@janosh janosh Aug 6, 2024

Choose a reason for hiding this comment

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

no, but i could have noticed that g.edges[u, v, k] is unlikely to be a dict in which case the first thought is to check whether the class in question implements __or__. (if not, |= is undefined behavior)


# optionally remove periodic image edges,
# these can be confusing due to periodic boundaries
Expand Down Expand Up @@ -2603,7 +2603,7 @@ def draw_graph_to_file(
d["label"] = f"{d['weight']:.2f} {units}"

# update edge with our new style attributes
g.edges[u, v, k] |= d
g.edges[u, v, k].update(d)

# optionally remove periodic image edges,
# these can be confusing due to periodic boundaries
Expand Down
44 changes: 25 additions & 19 deletions src/pymatgen/electronic_structure/boltztrap2.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@

if TYPE_CHECKING:
from pathlib import Path
from typing import Literal

from typing_extensions import Self

Expand Down Expand Up @@ -961,20 +962,20 @@ def __init__(self, bzt_transP=None, bzt_interp=None) -> None:

def plot_props(
self,
prop_y,
prop_x,
prop_z="temp",
output="avg_eigs",
dop_type="n",
doping=None,
temps=None,
xlim=(-2, 2),
ax: plt.Axes = None,
):
prop_y: str,
prop_x: Literal["mu", "doping", "temp"],
prop_z: Literal["doping", "temp"] = "temp",
output: Literal["avg_eigs", "eigs"] = "avg_eigs",
dop_type: Literal["n", "p"] = "n",
doping: list[float] | None = None,
temps: list[float] | None = None,
xlim: tuple[float, float] = (-2, 2),
ax: plt.Axes | None = None,
) -> plt.Axes | plt.Figure:
"""Plot the transport properties.

Args:
prop_y: property to plot among ("Conductivity","Seebeck","Kappa","Carrier_conc",
prop_y: property to plot among ("Conductivity", "Seebeck", "Kappa", "Carrier_conc",
"Hall_carrier_conc_trace"). Abbreviations are possible, like "S" for "Seebeck"
prop_x: independent variable in the x-axis among ('mu','doping','temp')
prop_z: third variable to plot multiple curves ('doping','temp')
Expand All @@ -991,7 +992,9 @@ def plot_props(
ax: figure.axes where to plot. If None, a new figure is produced.

Returns:
plt.Axes: matplotlib Axes object
plt.Axes: matplotlib Axes object if ax provided
OR
plt.Figure: matplotlib Figure object if ax is None

Example:
bztPlotter.plot_props('S','mu','temp',temps=[600,900,1200]).show()
Expand Down Expand Up @@ -1026,15 +1029,15 @@ def plot_props(
r"$(cm^{-3})$",
)

props_short = [p[: len(prop_y)] for p in props]
props_short = tuple(p[: len(prop_y)] for p in props)

if prop_y not in props_short:
raise BoltztrapError("prop_y not valid")

if prop_x not in ("mu", "doping", "temp"):
if prop_x not in {"mu", "doping", "temp"}:
raise BoltztrapError("prop_x not valid")

if prop_z not in ("doping", "temp"):
if prop_z not in {"doping", "temp"}:
raise BoltztrapError("prop_z not valid")

idx_prop = props_short.index(prop_y)
Expand All @@ -1048,8 +1051,7 @@ def plot_props(
else:
p_array = getattr(self.bzt_transP, f"{props[idx_prop]}_{prop_x}")

if ax is None:
plt.figure(figsize=(10, 8))
fig = plt.figure(figsize=(10, 8)) if ax is None else None

temps_all = self.bzt_transP.temp_r.tolist()
if temps is None:
Expand Down Expand Up @@ -1112,6 +1114,9 @@ def plot_props(
leg_title = f"{dop_type}-type"

elif prop_z == "doping" and prop_x == "temp":
if doping is None:
raise ValueError("doping cannot be None when prop_z is doping")

for dop in doping:
dop_idx = doping_all.index(dop)
prop_out = np.linalg.eigh(p_array[dop_type][:, dop_idx])[0]
Expand All @@ -1137,10 +1142,11 @@ def plot_props(
plt.ylabel(f"{props_lbl[idx_prop]} {props_unit[idx_prop]}", fontsize=30)
plt.xticks(fontsize=25)
plt.yticks(fontsize=25)
plt.legend(title=leg_title if leg_title != "" else "", fontsize=15)
plt.legend(title=leg_title or "", fontsize=15)
plt.tight_layout()
plt.grid()
return ax

return fig if ax is None else ax

def plot_bands(self):
"""Plot a band structure on symmetry line using BSPlotter()."""
Expand Down
30 changes: 15 additions & 15 deletions tests/command_line/test_bader_caller.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import warnings
from shutil import which
from unittest.mock import patch

import numpy as np
import pytest
Expand Down Expand Up @@ -60,7 +59,6 @@ def test_init(self):
assert len(analysis.data) == 14

# Test Cube file format parsing

copy_r(TEST_DIR, self.tmp_path)
analysis = BaderAnalysis(cube_filename=f"{TEST_DIR}/elec.cube.gz")
assert len(analysis.data) == 9
Expand All @@ -76,17 +74,17 @@ def test_from_path(self):
analysis = BaderAnalysis(chgcar_filename=chgcar_path, chgref_filename=chgref_path)
analysis_from_path = BaderAnalysis.from_path(from_path_dir)

for key in analysis_from_path.summary:
val, val_from_path = analysis.summary[key], analysis_from_path.summary[key]
if isinstance(analysis_from_path.summary[key], (bool, str)):
for key, val_from_path in analysis_from_path.summary.items():
val = analysis.summary[key]
if isinstance(val_from_path, (bool, str)):
assert val == val_from_path, f"{key=}"
elif key == "charge":
assert_allclose(val, val_from_path, atol=1e-5)

def test_bader_analysis_from_path(self):
summary = bader_analysis_from_path(TEST_DIR)
"""
Reference summary dict (with bader 1.0)

summary_ref = {
"magmom": [4.298761, 4.221997, 4.221997, 3.816685, 4.221997, 4.298763, 0.36292, 0.370516, 0.36292,
0.36292, 0.36292, 0.36292, 0.36292, 0.370516],
Expand All @@ -102,6 +100,9 @@ def test_bader_analysis_from_path(self):
"reference_used": True,
}
"""

summary = bader_analysis_from_path(TEST_DIR)

assert set(summary) == {
"magmom",
"min_dist",
Expand Down Expand Up @@ -131,12 +132,11 @@ def test_atom_parsing(self):
)

def test_missing_file_bader_exe_path(self):
pytest.skip("doesn't reliably raise RuntimeError")
# mock which("bader") to return None so we always fall back to use bader_exe_path
with (
patch("shutil.which", return_value=None),
pytest.raises(
RuntimeError, match="BaderAnalysis requires the executable bader be in the PATH or the full path "
),
):
BaderAnalysis(chgcar_filename=f"{VASP_OUT_DIR}/CHGCAR.Fe3O4.gz", bader_exe_path="")
# Mock which("bader") to return None so we always fall back to use bader_exe_path
with pytest.MonkeyPatch.context() as monkeypatch:
monkeypatch.setenv("PATH", "")

with pytest.raises(
RuntimeError, match="Requires bader or bader.exe to be in the PATH or the absolute path"
):
BaderAnalysis(chgcar_filename=f"{VASP_OUT_DIR}/CHGCAR.Fe3O4.gz")
2 changes: 2 additions & 0 deletions tests/electronic_structure/test_boltztrap2.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,9 @@ def test_plot(self):
assert self.bztPlotter is not None
fig = self.bztPlotter.plot_props("S", "mu", "temp", temps=[300, 500])
assert fig is not None

fig = self.bztPlotter.plot_bands()
assert fig is not None

fig = self.bztPlotter.plot_dos()
assert fig is not None
2 changes: 1 addition & 1 deletion tests/ext/test_cod.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

if "CI" in os.environ: # test is slow and flaky, skip in CI. see
# https://github.com/materialsproject/pymatgen/pull/3777#issuecomment-2071217785
pytest.skip(allow_module_level=True)
pytest.skip(allow_module_level=True, reason="Skip COD test in CI")

try:
website_down = requests.get("https://www.crystallography.net", timeout=600).status_code != 200
Expand Down