From ac0672d16e78aef081660cd82863a8fb2acaa83d Mon Sep 17 00:00:00 2001 From: larsbratholm Date: Thu, 4 Oct 2018 13:27:57 +0100 Subject: [PATCH 1/3] Minor formatting changes. Removed omp from a loop that didn't seem safe. --- qml/representations/frepresentations.f90 | 53 +++++++++++------------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/qml/representations/frepresentations.f90 b/qml/representations/frepresentations.f90 index 263ff6004..24089053b 100644 --- a/qml/representations/frepresentations.f90 +++ b/qml/representations/frepresentations.f90 @@ -35,22 +35,19 @@ subroutine get_indices(natoms, nuclear_charges, type1, n, type1_indices) integer, dimension(:), intent(out) :: type1_indices integer :: j - !$OMP PARALLEL DO REDUCTION(+:n) do j = 1, natoms if (nuclear_charges(j) == type1) then - ! this shouldn't be a race condition n = n + 1 type1_indices(n) = j endif enddo - !$OMP END PARALLEL DO end subroutine get_indices end module representations subroutine fgenerate_coulomb_matrix(atomic_charges, coordinates, nmax, cm) - + implicit none double precision, dimension(:), intent(in) :: atomic_charges @@ -364,23 +361,23 @@ subroutine fgenerate_local_coulomb_matrix(central_atom_indices, central_natoms, allocate(sorted_atoms_all(natoms, central_natoms)) !$OMP PARALLEL DO PRIVATE(k) - do l = 1, central_natoms - k = central_atom_indices(l) - row_norms(k,l) = huge_double - enddo + do l = 1, central_natoms + k = central_atom_indices(l) + row_norms(k,l) = huge_double + enddo !$OMP END PARALLEL DO !$OMP PARALLEL DO PRIVATE(j,k) - do l = 1, central_natoms - k = central_atom_indices(l) - !$OMP CRITICAL - do i = 1, cutoff_count(k) - j = maxloc(row_norms(:,l), dim=1) - sorted_atoms_all(i, l) = j - row_norms(j,l) = 0.0d0 - enddo - !$OMP END CRITICAL - enddo + do l = 1, central_natoms + k = central_atom_indices(l) + !$OMP CRITICAL + do i = 1, cutoff_count(k) + j = maxloc(row_norms(:,l), dim=1) + sorted_atoms_all(i, l) = j + row_norms(j,l) = 0.0d0 + enddo + !$OMP END CRITICAL + enddo !$OMP END PARALLEL DO ! Clean up @@ -541,11 +538,11 @@ subroutine fgenerate_atomic_coulomb_matrix(central_atom_indices, central_natoms, do l = 1, central_natoms k = central_atom_indices(l) !$OMP CRITICAL - do i = 1, cutoff_count(k) - j = minloc(distance_matrix_tmp(:,k), dim=1) - sorted_atoms_all(i, l) = j - distance_matrix_tmp(j, k) = huge_double - enddo + do i = 1, cutoff_count(k) + j = minloc(distance_matrix_tmp(:,k), dim=1) + sorted_atoms_all(i, l) = j + distance_matrix_tmp(j, k) = huge_double + enddo !$OMP END CRITICAL enddo !$OMP END PARALLEL DO @@ -735,12 +732,12 @@ subroutine fgenerate_bob(atomic_charges, coordinates, nuclear_charges, id, & n = 0 !$OMP PARALLEL DO REDUCTION(+:n) - do i = 1, nid - n = n + nmax(i) * (1 + nmax(i)) - do j = 1, i - 1 - n = n + 2 * nmax(i) * nmax(j) - enddo + do i = 1, nid + n = n + nmax(i) * (1 + nmax(i)) + do j = 1, i - 1 + n = n + 2 * nmax(i) * nmax(j) enddo + enddo !$OMP END PARALLEL DO if (n /= 2*ncm) then From 9316c1bc9233af69d7fd91e195cd57fe39732dd4 Mon Sep 17 00:00:00 2001 From: Lars Andersen Bratholm Date: Thu, 4 Oct 2018 14:09:02 +0100 Subject: [PATCH 2/3] Fixed omp issue that resulted in inconsistent sorting --- qml/representations/frepresentations.f90 | 51 +++++++++++++----------- test/test_energy_krr_atomic_cmat.py | 12 ++---- 2 files changed, 31 insertions(+), 32 deletions(-) diff --git a/qml/representations/frepresentations.f90 b/qml/representations/frepresentations.f90 index 24089053b..a5a57e1a0 100644 --- a/qml/representations/frepresentations.f90 +++ b/qml/representations/frepresentations.f90 @@ -220,6 +220,7 @@ subroutine fgenerate_local_coulomb_matrix(central_atom_indices, central_natoms, integer :: idx double precision, allocatable, dimension(:, :) :: row_norms + double precision, allocatable, dimension(:) :: row_norms_l double precision :: pair_norm double precision :: prefactor double precision :: norm @@ -300,15 +301,17 @@ subroutine fgenerate_local_coulomb_matrix(central_atom_indices, central_natoms, ! Allocate temporary allocate(pair_distance_matrix(natoms, natoms, central_natoms)) allocate(row_norms(natoms, central_natoms)) + allocate(row_norms_l(natoms)) pair_distance_matrix = 0.0d0 row_norms = 0.0d0 - !$OMP PARALLEL DO PRIVATE(pair_norm, prefactor, k) REDUCTION(+:row_norms) COLLAPSE(2) - do i = 1, natoms - do l = 1, central_natoms - k = central_atom_indices(l) + !$OMP PARALLEL DO PRIVATE(pair_norm, prefactor, k, row_norms_l) + do l = 1, central_natoms + k = central_atom_indices(l) + row_norms_l = 0.0d0 + do i = 1, natoms ! self interaction if (distance_matrix(i,k) > cent_cutoff) then cycle @@ -322,7 +325,7 @@ subroutine fgenerate_local_coulomb_matrix(central_atom_indices, central_natoms, pair_norm = prefactor * prefactor * 0.5d0 * atomic_charges(i) ** 2.4d0 pair_distance_matrix(i,i,l) = pair_norm - row_norms(i,l) = row_norms(i,l) + pair_norm * pair_norm + row_norms_l(i) = row_norms_l(i) + pair_norm * pair_norm do j = i+1, natoms if (distance_matrix(j,k) > cent_cutoff) then @@ -350,13 +353,17 @@ subroutine fgenerate_local_coulomb_matrix(central_atom_indices, central_natoms, pair_distance_matrix(i, j, l) = pair_norm pair_distance_matrix(j, i, l) = pair_norm pair_norm = pair_norm * pair_norm - row_norms(i,l) = row_norms(i,l) + pair_norm - row_norms(j,l) = row_norms(j,l) + pair_norm + row_norms_l(i) = row_norms_l(i) + pair_norm + row_norms_l(j) = row_norms_l(j) + pair_norm enddo enddo + row_norms(:,l) = row_norms_l enddo !$OMP END PARALLEL DO + ! Clean up + deallocate(row_norms_l) + ! Allocate temporary allocate(sorted_atoms_all(natoms, central_natoms)) @@ -370,13 +377,11 @@ subroutine fgenerate_local_coulomb_matrix(central_atom_indices, central_natoms, !$OMP PARALLEL DO PRIVATE(j,k) do l = 1, central_natoms k = central_atom_indices(l) - !$OMP CRITICAL - do i = 1, cutoff_count(k) - j = maxloc(row_norms(:,l), dim=1) - sorted_atoms_all(i, l) = j - row_norms(j,l) = 0.0d0 - enddo - !$OMP END CRITICAL + do i = 1, cutoff_count(k) + j = maxloc(row_norms(:,l), dim=1) + sorted_atoms_all(i, l) = j + row_norms(j,l) = 0.0d0 + enddo enddo !$OMP END PARALLEL DO @@ -389,17 +394,17 @@ subroutine fgenerate_local_coulomb_matrix(central_atom_indices, central_natoms, cm = 0.0d0 !$OMP PARALLEL DO PRIVATE(i, j, k, idx) - do l = 1, central_natoms - k = central_atom_indices(l) - do m = 1, cutoff_count(k) - i = sorted_atoms_all(m, l) - idx = (m*m+m)/2 - m - do n = 1, m - j = sorted_atoms_all(n, l) - cm(l, idx+n) = pair_distance_matrix(i,j,l) - enddo + do l = 1, central_natoms + k = central_atom_indices(l) + do m = 1, cutoff_count(k) + i = sorted_atoms_all(m, l) + idx = (m*m+m)/2 - m + do n = 1, m + j = sorted_atoms_all(n, l) + cm(l, idx+n) = pair_distance_matrix(i,j,l) enddo enddo + enddo !$OMP END PARALLEL DO diff --git a/test/test_energy_krr_atomic_cmat.py b/test/test_energy_krr_atomic_cmat.py index b5940f50a..fd69cae84 100644 --- a/test/test_energy_krr_atomic_cmat.py +++ b/test/test_energy_krr_atomic_cmat.py @@ -124,11 +124,8 @@ def test_krr_gaussian_local_cmat(): Ks = get_local_kernels_gaussian(Xs, X, Ns, N, [sigma])[0] Ks_test = np.loadtxt(test_dir + "/data/Ks_local_gaussian.txt") - # Somtimes a few coulomb matrices differ because of parallel sorting and numerical error - # Allow up to 5 molecules to differ from the supplied reference. - differences_count = len(set(np.where(Ks - Ks_test > 1e-7)[0])) - assert differences_count < 5, "Error in local Laplacian kernel (vs. reference)" - # assert np.allclose(Ks, Ks_test), "Error in local Gaussian kernel (vs. reference)" + + assert np.allclose(Ks, Ks_test), "Error in local Gaussian kernel (vs. reference)" Ks_test = get_atomic_kernels_gaussian(test, training, [sigma])[0] assert np.allclose(Ks, Ks_test), "Error in local Gaussian kernel (vs. wrapper)" @@ -207,10 +204,7 @@ def test_krr_laplacian_local_cmat(): Ks_test = np.loadtxt(test_dir + "/data/Ks_local_laplacian.txt") - # Somtimes a few coulomb matrices differ because of parallel sorting and numerical error - # Allow up to 5 molecules to differ from the supplied reference. - differences_count = len(set(np.where(Ks - Ks_test > 1e-7)[0])) - assert differences_count < 5, "Error in local Laplacian kernel (vs. reference)" + assert np.allclose(Ks, Ks_test), "Error in local Laplacian kernel (vs. reference)" Ks_test = get_atomic_kernels_laplacian(test, training, [sigma])[0] assert np.allclose(Ks, Ks_test), "Error in local Laplacian kernel (vs. wrapper)" From 12bc8d9da9138afa87e8c43a5b75b13d012ad3cd Mon Sep 17 00:00:00 2001 From: larsbratholm Date: Thu, 4 Oct 2018 18:20:46 +0100 Subject: [PATCH 3/3] Revert changes to test. Local builds pass but travis fail, but I assume that the changes made should be more robust --- test/test_energy_krr_atomic_cmat.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/test_energy_krr_atomic_cmat.py b/test/test_energy_krr_atomic_cmat.py index fd69cae84..b5940f50a 100644 --- a/test/test_energy_krr_atomic_cmat.py +++ b/test/test_energy_krr_atomic_cmat.py @@ -124,8 +124,11 @@ def test_krr_gaussian_local_cmat(): Ks = get_local_kernels_gaussian(Xs, X, Ns, N, [sigma])[0] Ks_test = np.loadtxt(test_dir + "/data/Ks_local_gaussian.txt") - - assert np.allclose(Ks, Ks_test), "Error in local Gaussian kernel (vs. reference)" + # Somtimes a few coulomb matrices differ because of parallel sorting and numerical error + # Allow up to 5 molecules to differ from the supplied reference. + differences_count = len(set(np.where(Ks - Ks_test > 1e-7)[0])) + assert differences_count < 5, "Error in local Laplacian kernel (vs. reference)" + # assert np.allclose(Ks, Ks_test), "Error in local Gaussian kernel (vs. reference)" Ks_test = get_atomic_kernels_gaussian(test, training, [sigma])[0] assert np.allclose(Ks, Ks_test), "Error in local Gaussian kernel (vs. wrapper)" @@ -204,7 +207,10 @@ def test_krr_laplacian_local_cmat(): Ks_test = np.loadtxt(test_dir + "/data/Ks_local_laplacian.txt") - assert np.allclose(Ks, Ks_test), "Error in local Laplacian kernel (vs. reference)" + # Somtimes a few coulomb matrices differ because of parallel sorting and numerical error + # Allow up to 5 molecules to differ from the supplied reference. + differences_count = len(set(np.where(Ks - Ks_test > 1e-7)[0])) + assert differences_count < 5, "Error in local Laplacian kernel (vs. reference)" Ks_test = get_atomic_kernels_laplacian(test, training, [sigma])[0] assert np.allclose(Ks, Ks_test), "Error in local Laplacian kernel (vs. wrapper)"