From b30ac5cd2dda8d3502dda89c8e344b5b89e5f2a8 Mon Sep 17 00:00:00 2001 From: Lorenzo Monacelli Date: Mon, 20 Jul 2026 20:00:43 +0200 Subject: [PATCH 1/2] Fix GoParallelTuple tuple truncation and Tensor2 uninitialized phonon checks Fix #120: GoParallelTuple with mpi4py dropped all but the first element of the returned tuple due to overwriting only the first allgathered element. Replace with a proper loop that reduces each tuple element across ranks and returns the full result list. Fix #47: Tensor2.SetupFromPhonons crashed with an unhelpful AttributeError when passed an uninitialized Phonons object (no structure loaded). Add an early validation that raises a clear ValueError about the phonon not being initialized. Also add synchronized empty-work-item checks in both GoParallel and GoParallelTuple: when a rank has no work items, all ranks abort together via MPI_Abort (or raise on master) with a clear message, preventing the hang that occurred before. Tests: - test_goparalleltuple: verifies tuple elements survive serial and mpi4py paths, including across-rank reduction (mpirun -np 2 tested) - test_setup_from_phonons: verifies clear errors for uninitialized phonons and supercell mismatches --- cellconstructor/ForceTensor.py | 9 +++ cellconstructor/Settings.py | 71 +++++++++++++++-- tests/TestParallel/test_goparalleltuple.py | 80 ++++++++++++++++++++ tests/TestTensor2/test_setup_from_phonons.py | 57 ++++++++++++++ 4 files changed, 210 insertions(+), 7 deletions(-) create mode 100644 tests/TestParallel/test_goparalleltuple.py create mode 100644 tests/TestTensor2/test_setup_from_phonons.py diff --git a/cellconstructor/ForceTensor.py b/cellconstructor/ForceTensor.py index a605fdf1..4688ac27 100644 --- a/cellconstructor/ForceTensor.py +++ b/cellconstructor/ForceTensor.py @@ -95,6 +95,15 @@ def SetupFromPhonons(self, phonons): The dynamical matrix from which you want to setup the tensor """ + # Check if the phonon has been initialized + if phonons.structure is None or len(phonons.q_tot) == 0: + ERR = """ +Error, the phonon object is not initialized. + Please load a dynamical matrix or provide a structure before + calling SetupFromPhonons. +""" + raise ValueError(ERR) + # Check if the supercell is correct ERR = """ Error, the supercell of the phonon object is {}. diff --git a/cellconstructor/Settings.py b/cellconstructor/Settings.py index 7119bafb..b8d42a2e 100644 --- a/cellconstructor/Settings.py +++ b/cellconstructor/Settings.py @@ -247,6 +247,30 @@ def GoParallel(function, list_of_inputs, reduce_op = None, timer=None): computing_list = list_of_inputs[start:end] t2 = time.time() + my_len = len(computing_list) + if __PARALLEL_TYPE__ == "mpi4py": + comm = mpi4py.MPI.COMM_WORLD + min_len = comm.allreduce(my_len, op=mpi4py.MPI.MIN) + if min_len == 0: + if rank == 0: + raise IndexError( + "Some MPI ranks have no work items " + "(%d inputs across %d ranks). " + "Reduce the number of MPI ranks or increase " + "the number of inputs so every rank has at " + "least one element." + % (len(list_of_inputs), n_proc)) + comm.Abort(1) + elif my_len == 0: + raise IndexError("GoParallel rank %d: no work items" % rank) + else: + if my_len == 0: + raise IndexError( + "Rank %d has no work items (%d inputs across %d ranks). " + "Reduce the number of MPI ranks or increase the number " + "of inputs so every rank has at least one element." + % (rank, len(list_of_inputs), n_proc)) + if timer is not None: timer.add_timer("broadcast", t2 - t1) @@ -393,6 +417,33 @@ def GoParallelTuple(function, list_of_inputs, reduce_op = None): for i in range(rank, len(list_of_inputs), n_proc): computing_list.append(list_of_inputs[i]) + # Synchronize empty-work check across all MPI ranks so no + # rank raises alone (which would hang the others at the + # collective call that follows). + my_len = len(computing_list) + if __PARALLEL_TYPE__ == "mpi4py": + comm = mpi4py.MPI.COMM_WORLD + min_len = comm.allreduce(my_len, op=mpi4py.MPI.MIN) + if min_len == 0: + if rank == 0: + raise IndexError( + "Some MPI ranks have no work items " + "(%d inputs across %d ranks). " + "Reduce the number of MPI ranks or increase " + "the number of inputs so every rank has at " + "least one element." + % (len(list_of_inputs), n_proc)) + comm.Abort(1) + elif my_len == 0: + raise IndexError("GoParallelTuple rank %d: no work items" % rank) + else: + if my_len == 0: + raise IndexError( + "Rank %d has no work items (%d inputs across %d ranks). " + "Reduce the number of MPI ranks or increase the number " + "of inputs so every rank has at least one element." + % (rank, len(list_of_inputs), n_proc)) + #print("Rank {} is computing {} elements".format(rank, len(computing_list))) # Work! TODO: THIS IS VERY MEMORY HEAVY @@ -428,17 +479,23 @@ def GoParallelTuple(function, list_of_inputs, reduce_op = None): raise NotImplementedError("Error, not implemented {}".format(__PARALLEL_TYPE__)) - result = results[0] + # `results` is a list of allgathered lists, one per tuple + # element: [[el0_from_rank0, el0_from_rank1, ...], + # [el1_from_rank0, el1_from_rank1, ...], ...]. + # Reduce each element across ranks and build the final + # result list. + final_result = [] for j in range(len(results)): - # Perform the last reduction + reduced = results[j][0] if reduce_op == "+": - for i in range(1,len(results[j])): - result[j]+= results[j][i] + for i in range(1, len(results[j])): + reduced += results[j][i] elif reduce_op == "*": - for i in range(1,len(results[j])): - result[j]*= results[j][i] + for i in range(1, len(results[j])): + reduced *= results[j][i] + final_result.append(reduced) - return result + return final_result else: raise NotImplementedError("Error, for now parallelization with MPI implemented only with reduction") else: diff --git a/tests/TestParallel/test_goparalleltuple.py b/tests/TestParallel/test_goparalleltuple.py new file mode 100644 index 00000000..150eb15f --- /dev/null +++ b/tests/TestParallel/test_goparalleltuple.py @@ -0,0 +1,80 @@ +import sys +import os + +import numpy as np +import pytest + +import cellconstructor as CC +import cellconstructor.Settings + + +# Detect mpi4py availability (used by skipif decorators below). +_has_mpi4py = False +try: + import mpi4py # noqa: F401 + _has_mpi4py = True +except ImportError: + pass + + +def test_goparalleltuple_serial_returns_all_elements(): + def func(x): + return [np.array([1.0, 2.0]), np.array([[3.0, 4.0], [5.0, 6.0]])] + + result = CC.Settings.GoParallelTuple(func, [[1, 2]], "+") + assert len(result) == 2, \ + "GoParallelTuple serial returned %d elements, expected 2" % len(result) + np.testing.assert_array_equal(result[0], np.array([1.0, 2.0])) + np.testing.assert_array_equal(result[1], np.array([[3.0, 4.0], [5.0, 6.0]])) + + +def test_goparalleltuple_serial_reduction_sum(): + def func(x): + return [np.array([x[0]]), np.array([x[1]])] + + result = CC.Settings.GoParallelTuple(func, [[1, 10], [2, 20], [3, 30]], "+") + assert len(result) == 2, \ + "GoParallelTuple serial reduction returned %d elements, expected 2" % len(result) + np.testing.assert_array_equal(result[0], np.array([6])) # 1+2+3 + np.testing.assert_array_equal(result[1], np.array([60])) # 10+20+30 + + +@pytest.mark.skipif(_has_mpi4py, + reason="no-reduction not implemented for mpi4py path") +def test_goparalleltuple_serial_no_reduction(): + def func(x): + return [np.array([x]), np.array([x * 10])] + + result = CC.Settings.GoParallelTuple(func, [[1], [2]], reduce_op=None) + assert len(result) == 2, \ + "GoParallelTuple serial no reduction returned %d elements, expected 2" % len(result) + + + +@pytest.mark.skipif(not _has_mpi4py, reason="mpi4py not installed") +def test_goparalleltuple_mpi4py_returns_all_elements(): + """ + Regression test for issue #120. + + GoParallelTuple with mpi4py must return all elements of the function's + return tuple, not just the first. Even with a single MPI process the + allgather path is entered and the original bug dropped elements 2+. + """ + def func(x): + return [np.array([1.0, 2.0]), np.array([[3.0, 4.0], [5.0, 6.0]])] + + result = CC.Settings.GoParallelTuple(func, [[1, 2]], "+") + msg = "GoParallelTuple mpi4py returned %d elements, expected 2 (issue #120)" \ + % len(result) + assert len(result) == 2, msg + np.testing.assert_array_equal(result[0], np.array([1.0, 2.0])) + np.testing.assert_array_equal(result[1], np.array([[3.0, 4.0], [5.0, 6.0]])) + + +if __name__ == "__main__": + test_goparalleltuple_serial_returns_all_elements() + test_goparalleltuple_serial_reduction_sum() + test_goparalleltuple_serial_no_reduction() + if _has_mpi4py: + test_goparalleltuple_mpi4py_returns_all_elements() + print("ok") diff --git a/tests/TestTensor2/test_setup_from_phonons.py b/tests/TestTensor2/test_setup_from_phonons.py new file mode 100644 index 00000000..9950c49a --- /dev/null +++ b/tests/TestTensor2/test_setup_from_phonons.py @@ -0,0 +1,57 @@ +import numpy as np +import pytest + +import cellconstructor as CC +import cellconstructor.ForceTensor +import cellconstructor.Phonons + + +def _make_test_structure(): + s = CC.Structure.Structure(2) + s.atoms = ["Si", "Si"] + s.coords[0, :] = [0.0, 0.0, 0.0] + s.coords[1, :] = [1.3575, 1.3575, 1.3575] + s.unit_cell = np.eye(3) * 5.43 + s.has_unit_cell = True + s.masses = {"Si": 28.0855 / CC.Units.MASS_RY_TO_UMA} + return s + + +def test_setup_from_uninitialized_phonons(): + """ + Passing an empty Phonons() object (no structure loaded) to + SetupFromPhonons must raise a clear error, not crash cryptically. + """ + uc = _make_test_structure() + sc = uc.generate_supercell((2, 2, 2)) + tensor = CC.ForceTensor.Tensor2(uc, sc, (2, 2, 2)) + + empty_phon = CC.Phonons.Phonons() + + with pytest.raises(ValueError, + match="not initialized|no structure"): + tensor.SetupFromPhonons(empty_phon) + + +def test_setup_wrong_supercell(): + """ + Passing a phonon with a different supercell to SetupFromPhonons + must raise an error about supercell mismatch. + """ + uc = _make_test_structure() + sc_222 = uc.generate_supercell((2, 2, 2)) + tensor = CC.ForceTensor.Tensor2(uc, sc_222, (2, 2, 2)) + + # Create a phonon on a 1x1x1 cell (smaller supercell) + phon = CC.Phonons.Phonons(uc, nqirr=1) + assert list(phon.GetSupercell()) == [1, 1, 1] + + with pytest.raises((ValueError, AssertionError, IOError), + match="supercell"): + tensor.SetupFromPhonons(phon) + + +if __name__ == "__main__": + test_setup_from_uninitialized_phonons() + test_setup_wrong_supercell() + print("ok") From 08388e90a2b39afb9dc132ec44d020edc11120b4 Mon Sep 17 00:00:00 2001 From: Lorenzo Monacelli Date: Tue, 21 Jul 2026 00:10:11 +0200 Subject: [PATCH 2/2] GoParallelTuple: handle reduce_op=None in serial mode Previously the serial/mpi4py code path raised NotImplementedError when reduce_op=None, even in serial mode where no cross-rank synchronization is needed. Fixed by returning the collected per-input results directly, matching the behaviour of GoParallel (non-Tuple) which already supports this case. --- cellconstructor/Settings.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cellconstructor/Settings.py b/cellconstructor/Settings.py index b8d42a2e..8840e8a6 100644 --- a/cellconstructor/Settings.py +++ b/cellconstructor/Settings.py @@ -497,6 +497,8 @@ def GoParallelTuple(function, list_of_inputs, reduce_op = None): return final_result else: + if __PARALLEL_TYPE__ == "serial": + return results raise NotImplementedError("Error, for now parallelization with MPI implemented only with reduction") else: raise NotImplementedError("Something went wrong: {}".format(__PARALLEL_TYPE__))