Repository navigation
Conversation
… cell list heuristic The per-center Python loop in rdf() dominated the runtime, even for large systems where the cell list approach should pay off. Replace it with a single vectorized displacement evaluation over the pairs returned by the cell list query. For small workloads, i.e. few centers or few atoms, the cell list overhead does not pay off. Add an optional use_cell_list parameter, that allows to explicitly choose the approach, or let it be chosen automatically from a work estimate based on the number of centers, the number of atoms and the fraction of the system volume that the cell list queries visit. Fixes biotite-dev#307
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rdf()evaluated the displacement for each center individually in a Python loop, even though each iteration only selected rows from the cell list query result. This dominated the runtime for all system sizes (see benchmarks below).This PR
displacement()evaluation over the(center, atom)pairs fromCellList.get_atoms_in_cells(..., result_format=CellList.Result.PAIRS). Forperiodic=Truethe periodic image duplicates in the raw pairs are removed after mapping the image indices back to the original atoms.use_cell_listparameter that [WIP] Optional brute force approach for RDF calculation #315 introduced for the old pure-Python implementation, including the automatic choice proposed in Improve distance calculation speed for small systems #307: a work estimate compares the brute-force costn_centers * n_atomswith the cell list costn_atoms + factor * n_centers * n_atoms * visited_fraction, where the visited fraction is estimated from the ratio of the swept cell volume((3 * interval_max)**3)to the system volume. For few centers or small systems the brute-force approach evaluates the full broadcast instead.periodic=True: in that regime minimum-image displacements and cell-list preselection are not guaranteed to be interchangeable for strongly sheared triclinic boxes, so the automatic mode conservatively uses the cell list.This supersedes #315 (written against the pre-Rust implementation) and closes #307.
Correctness
The distance evaluation per pair is unchanged —
displacement()is called with the same coordinates, the sameboxand the same minimum-image convention as before; only the batching changed, andnp.histogramcounting is order-independent.Verified against
mainwith a test set of 9 cases (single/multiple centers, periodic/non-periodic, multi-model stacks,selection, adversarial small box with interval beyond the inradius, strongly sheared triclinic boxes): output bins andg_rare exactly identical (np.array_equal) tomainfor all cases, for both explicitly chosen paths and the automatic choice.New tests in
tests/structure/test_rdf.py:test_rdf_use_cell_list_equivalence: cell list and brute-force paths give identical results (periodic and non-periodic).test_rdf_small_system_heuristic: the automatic choice matches an explicitly chosen path for a single center.Benchmarks
macOS (M-series laptop), Python 3.13, NumPy 2.x, median of 7 runs,
bins=100, interval=(0, 10), fixed seed:main*The synthetic sparse-box rows include the heuristic overhead: the crossover from brute-force to cell list sits between 10 and 100 centers for this density; the per-component cost at 100 centers is 5.4 ms (cell list) vs 73 ms (brute force), i.e. the heuristic picks the cheaper path with an order of magnitude margin.
The interesting small-system regime (few centers,
use_cell_list=Falseexplicitly) is 1.5-2x faster thanmain:main(loop over cell list)Added
benchmarks/structure/benchmark_rdf.py(few/many centers, small/large/sparse systems) for CodSpeed tracking.Testing
pytest tests/structure/test_rdf.py— 9 passed (7 existing + 2 new)pytest tests/structure/ --ignore=tests/structure/io --ignore=tests/structure/test_connect.py --ignore=tests/structure/test_info.py: failure set identical tomain(306 pre-existing failures/errors from missing optional data, 0 regressions)ruff check/ruff format --checkon touched files: cleanpyrightonrdf.py: same pre-existing findings asmain, no new onesFixes #307
Supersedes #315