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

Add CI pipeline #28

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
47 changes: 47 additions & 0 deletions .github/workflows/python-app.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
name: Test NeuralForceField package

on: [push]

jobs:
build:

runs-on: ubuntu-latest
strategy:
matrix:
# python-version: ["pypy3.10", "3.8", "3.9", "3.10", "3.11", "3.12", "3.13"]
python-version: ["3.10"]

steps:
- uses: actions/checkout@v4
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
- name: Display Python version
run: python -c "import sys; print(sys.version)"
- name: Install basics
run: python -m pip install --upgrade pip setuptools wheel
- name: Install package
run: python -m pip install .
# - name: Install linters
# run: python -m pip install flake8 mypy pylint
# - name: Install documentation requirements
# run: python -m pip install -r docs/requirements.txt
# - name: Test with flake8
# run: flake8 polymethod
# - name: Test with mypy
# run: mypy polymethod
# - name: Test with pylint
# run: pylint polymethod
- name: Test with pytest
run: |
pip install pytest pytest-cov
pytest nff/tests --doctest-modules --junitxml=junit/test-results-${{ matrix.python-version }}.xml --cov=nff --cov-report=xml --cov-report=html
- name: Upload pytest test results
uses: actions/upload-artifact@v4
with:
name: pytest-results-${{ matrix.python-version }}
path: junit/test-results-${{ matrix.python-version }}.xml
if: ${{ always() }}
# - name: Test documentation
# run: sphinx-build docs/source docs/build
12 changes: 12 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,17 @@ dist/
sandbox_excited/
build/

# Editor files
# vim
*.swp
*.swo

# pycharm
.idea/

# coverage and tests
junit
.coverage

# required exceptions
!tutorials/models/ammonia/Ammonia.xyz
5 changes: 5 additions & 0 deletions nff/data/dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def __init__(
units: str = "kcal/mol",
check_props: bool = True,
do_copy: bool = True,
device: str = "cuda"
) -> None:
"""Constructor for Dataset class.

Expand All @@ -108,6 +109,7 @@ def __init__(
self.props = props
self.units = units
self.to_units(units)
self.device = device

def __len__(self) -> int:
"""Length of the dataset.
Expand Down Expand Up @@ -289,6 +291,7 @@ def _get_periodic_neighbor_list(
pbc=True,
cutoff=cutoff,
directed=(not undirected),
device=self.device,
)
nbrs, offs = atoms.update_nbr_list()
nbrlist.append(nbrs)
Expand Down Expand Up @@ -444,6 +447,7 @@ def unwrap_xyz(self, mol_dic: dict) -> None:
numbers=self.props["nxyz"][i][:, 0],
cell=self.props["cell"][i],
pbc=True,
device=self.device
)

# recontruct coordinates based on subgraphs index
Expand Down Expand Up @@ -577,6 +581,7 @@ def gen_bond_prior(self, cutoff: float, bond_len_dict: dict | None = None) -> No
"cutoff": cutoff,
"cell": cell,
"nbr_torch": False,
"device": self.device
}

# the coordinates have been unwrapped and try to results offsets
Expand Down
File renamed without changes.
15 changes: 15 additions & 0 deletions nff/tests/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@

import os
import pytest
import torch

torch.set_num_threads(int(os.getenv("OMP_NUM_THREADS", 1)))


def pytest_addoption(parser):
parser.addoption("--device", action="store", default="cpu", help="Whether to use the CPU or GPU for the tests")


@pytest.fixture
def device(request):
return request.config.getoption("--device")
Binary file added nff/tests/data/azo_diabat.pth.tar
Binary file not shown.
Binary file added nff/tests/data/dataset.pth.tar
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
from ase.io.trajectory import Trajectory
from ase import Atoms

from nff.md.utils import mol_dot, mol_norm, ZhuNakamuraLogger, atoms_to_nxyz
from nff.md.nvt_test import NoseHoover, NoseHooverChain
from nff.md.utils_ax import mol_dot, mol_norm, ZhuNakamuraLogger, atoms_to_nxyz
from nff.md.nvt_ax import NoseHoover, NoseHooverChain
Comment on lines +18 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that we want to make this change... I believe the _ax files are modified versions from a former student (Simon Axelrod) and they may be out-of-date.

This duplicate code is one of many issues in NFF... I remember at one point that Simon suggested doing away with those versions entirely or integrating them with the main files, but they work for chemistry I'm less familiar with (excited states) and Simon didn't have time to migrate and update the code himself. I think I will open a specific issue and see if I can recruit him to check changes that we make.

Copy link
Author

Choose a reason for hiding this comment

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

I suspected something like we have already discussed concerning deprecated code might be going on with these _ax files. I anyway changed the import here, because

  1. some of the imported objects do not exist in the files, so the current import statement is broken
  2. it is just a test, so we do not change actual production runs.

If this test ever makes any issues, because the _ax files make some problems or we remove them, I would say we can look into removing this test or adapting it to the current code base?

from nff.utils.constants import BOHR_RADIUS, FS_TO_AU, AMU_TO_AU, FS_TO_ASE, ASE_TO_FS, EV_TO_AU
from nff.data import Dataset, collate_dicts
from nff.utils.cuda import batch_to
Expand Down
36 changes: 27 additions & 9 deletions nff/io/tests/test_ase.py → nff/tests/test_ase.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import numpy as np
from ase import Atoms

import pytest

Comment on lines +8 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be a bit out of touch, but is there an advantage to using pytest over unittest? I normally just use unittest. While I know that pytest serves a similar function and I've heard of it, I'm unfamiliar with formatting and syntax differences between the two and which integrates better with GitHub CI/CD.

Copy link
Author

Choose a reason for hiding this comment

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

I never looked into it too much, because it "just works" and pytest is the most commonly used testing system, but to my understanding the advantages are:

  1. Execution is quite simple, pytest follows an easy rule which functions in which files to execute and executes just these functions, so there is no extra code in the test files necessary
  2. Pytest integrates very well with easily available coverage reports
  3. In our case we have the CPU/GPU problem with most of the code base and pytest offers a not super easy, but relatively easy way to adjust variables in tests based on user settings when executing pytest. So in this PR I also add that you can do pytest --device cpu or pytest --device cuda to execute the tests either with CPU or GPU. CPU is the default, because it is easier on the CI. So far this is not documented, but I would focus on documentation / readme after linting the code base, but definitely worth putting on a list

from nff.io.ase import AtomsBatch


Expand All @@ -19,6 +21,8 @@ def compare_dicts(d1: dict, d2: dict):
for key, value in d1.items():
if isinstance(value, dict):
compare_dicts(value, d2[key])
elif isinstance(value, str):
assert value == d2[key]
elif isinstance(value, Iterable):
assert np.allclose(value, d2[key])
else:
Expand Down Expand Up @@ -47,10 +51,17 @@ def get_ethanol():
return Atoms(nxyz[:, 0].astype(int), positions=nxyz[:, 1:])


# @ut.skip("skip this for now")
@pytest.mark.usefixtures("device") # Ensure the fixture is accessible
class TestAtomsBatch(ut.TestCase):
def setUp(self):
self.ethanol = get_ethanol()
# Access the device value from the pytest fixture
self.device = self._test_fixture_device

@pytest.fixture(autouse=True)
def inject_device(self, device):
# Automatically set the fixture value to an attribute
self._test_fixture_device = device

@ut.skip("skip this for now")
def test_AtomsBatch(self):
Expand Down Expand Up @@ -111,7 +122,7 @@ def test_AtomsBatch(self):
]
)

atoms_batch = AtomsBatch(self.ethanol, cutoff=2.5)
atoms_batch = AtomsBatch(self.ethanol, cutoff=2.5, device=self.device)
atoms_batch.update_nbr_list()

G1 = nx.from_edgelist(expected_nbrlist_cutoff_2dot5)
Expand All @@ -120,21 +131,21 @@ def test_AtomsBatch(self):
assert nx.is_isomorphic(G1, G2)

def test_get_batch(self):
atoms_batch = AtomsBatch(self.ethanol, cutoff=5)
atoms_batch = AtomsBatch(self.ethanol, cutoff=5, device=self.device)
batch = atoms_batch.get_batch()

assert "nxyz" in batch

def test_from_atoms(self):
atoms_batch = AtomsBatch.from_atoms(self.ethanol, cutoff=2.5)
atoms_batch = AtomsBatch.from_atoms(self.ethanol, cutoff=2.5, device=self.device)

# ensure atomic numbers, positions, and cell are the same
assert np.allclose(atoms_batch.get_atomic_numbers(), self.ethanol.get_atomic_numbers())
assert np.allclose(atoms_batch.get_positions(), self.ethanol.get_positions())
assert np.allclose(atoms_batch.get_cell(), self.ethanol.get_cell())

def test_copy(self):
atoms_batch = AtomsBatch(self.ethanol, cutoff=2.5)
atoms_batch = AtomsBatch(self.ethanol, cutoff=2.5, device=self.device)
atoms_batch.get_batch() # update props
atoms_batch_copy = atoms_batch.copy()

Expand All @@ -154,7 +165,7 @@ def test_copy(self):
assert atoms_batch.requires_large_offsets == atoms_batch_copy.requires_large_offsets

def test_fromdict(self):
atoms_batch = AtomsBatch(self.ethanol, cutoff=2.5)
atoms_batch = AtomsBatch(self.ethanol, cutoff=2.5, device=self.device)
ab_dict = atoms_batch.todict(update_props=True)
ab_from_dict = AtomsBatch.fromdict(ab_dict)

Expand Down Expand Up @@ -183,6 +194,7 @@ def test_fromdict(self):
compare_dicts(ab_dict_props, ab_dict_again_props)


@pytest.mark.usefixtures("device") # Ensure the fixture is loaded
class TestPeriodic(ut.TestCase):
def setUp(self):
nxyz = np.array(
Expand All @@ -205,9 +217,15 @@ def setUp(self):
[0.0, 0.0, 5.51891759],
]
)
self.quartz = AtomsBatch(nxyz[:, 0].astype(int), positions=nxyz[:, 1:], cell=lattice, pbc=True)

def test_ase(self):
self.quartz = AtomsBatch(nxyz[:, 0].astype(int), positions=nxyz[:, 1:], cell=lattice, pbc=True,
device=self._test_fixture_device)

@pytest.fixture(autouse=True)
def inject_device(self, device):
# Automatically set the fixture value to an attribute
self._test_fixture_device = device

def test_print(self):
print(self.quartz)

def test_nbrlist(self):
Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import numpy as np
import torch

import pytest

from nff.data.dataset import (
Dataset,
concatenate_dict,
Expand All @@ -14,8 +16,8 @@
)

current_path = Path(__file__).parent
DATASET_PATH = current_path / "../../../tutorials/data/dataset.pth.tar"
PEROVSKITE_DATA_PATH = current_path / "./data/SrIrO3_bulk_55_nff_all_dataset.pth.tar"
DATASET_PATH = os.path.join(current_path, "..", "..", "..", "tutorials", "data", "dataset.pth.tar")
PEROVSKITE_DATA_PATH = os.path.join(current_path, "data", "SrIrO3_bulk_55_nff_all_dataset.pth.tar")
TARG_NAME = "formula"
VAL_SIZE = 0.1
TEST_SIZE = 0.1
Expand Down Expand Up @@ -223,6 +225,7 @@ def test_inexistent_list_lists(self):
self.assertEqual(ab, expected)


@pytest.mark.usefixtures("device") # Ensure the fixture is accessible
class TestPeriodicDataset(unittest.TestCase):
def setUp(self):
self.quartz = {
Expand All @@ -248,7 +251,12 @@ def setUp(self):
),
}

self.qtz_dataset = Dataset(concatenate_dict(*[self.quartz] * 3))
self.qtz_dataset = Dataset(concatenate_dict(*[self.quartz] * 3), device=self._test_fixture_device)

@pytest.fixture(autouse=True)
def inject_device(self, device):
# Automatically set the fixture value to an attribute
self._test_fixture_device = device

def test_neighbor_list(self):
nbrs, offs = self.qtz_dataset.generate_neighbor_list(cutoff=5)
Expand Down
File renamed without changes.
Loading