Optimization - #6
Open
Ajax23 wants to merge 39 commits into
Open
Conversation
- Remove old built HTML and RST source tree (docs/ built output, docsrc/) replaced by MyST Markdown sources in the new docs/ layout - Add tests/test_workflow.py: end-to-end PoreMS → PoreSim → PoreAna workflow test covering yml structure, PoreSim Box API, and PoreAna density/gyration/ file_to_text driven by a PoreMS-generated pore.yml - conftest.py: remove redundant parallel MC run that overwrote the serial output; reduce parallel MC comparison to the 7 lag steps actually checked by test_parallel_sample; run those serially to avoid spawn-mode pool hangs under Python 3.12 macOS pytest
Convert two is_parallel=True calls to is_parallel=False: the angle sample call in test_sample and the MC().run call in test_diffusion_mc_mc. Python 3.12 macOS uses spawn-mode multiprocessing which hangs under pytest for these workloads; serial execution is sufficient for correctness testing.
sample.py and mc.py: replace mp.Pool() with mp.get_context('fork').Pool()
on Linux and macOS. Python 3.12 changed the default start method on macOS
from fork to spawn; spawn causes hang-under-pytest issues due to heavy
re-import overhead per worker. Explicitly requesting fork restores the
original behaviour and is consistent with the Linux cluster environment
where the code primarily runs.
Windows falls back to the default (spawn), which is the only option there.
test_integration.py: MC().run() parallel test kept serial (is_parallel=False)
to avoid fork copy-on-write overhead on large model objects in CI. The
parallel sampling path (sample.sample is_parallel=True) is tested via the
conftest fixture and test_parallel_sample.
Drop Python 3.10 and 3.11 from CI matrix and classifiers; raise python_requires to >=3.12 in setup.py; update README and changelog.
Replace setup.py and MANIFEST.in with a single pyproject.toml.
Declare pytest and pytest-cov under [project.optional-dependencies] dev so the full test environment is installed with: pip install -e ".[dev]" CI updated to use the same extras group.
Remove three commented-out blocks from mc.py: stale save comment, incomplete txt-export stub, and unfinished radial diffusion algorithm (including junk debug line). Fix ajax23.github.io URL in further_props.md.
Replace manual venv setup with the dev install step so the README reflects the current pyproject.toml optional-dependencies setup.
Re-run all sampling benchmarks (3 passes, mean taken); parallel timings
drop from ~1.3-1.6 s to 0.10-0.14 s after the mp.get_context("fork") fix
for Python 3.12 macOS. Document the spawn-vs-fork root cause in changelog.
Administrative section now reflects pyproject.toml and the new [project.optional-dependencies] dev group.
- Replace all % string formatting with f-strings in diffusion.py print statements and utils.py file_to_text output - Replace range(len(lst)) anti-patterns with direct iteration (len_step, lagtime_inverse, D_mean) and zip() for parallel list operations in diffusion.py and freeenergy.py - Add encoding="utf-8" to all text-mode open() calls in utils.py for cross-platform correctness
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Brings in radial VACF support for cylindrical pores and the mc_profile is_legend parameter from the vacf branch; resolves all conflicts in favour of the optimization branch's modernized style.
Remove unused variable assignments from density.py and the VACF functions in diffusion.py; fix F821 by replacing bare `pa.density` calls with a local `_density` import; apply ruff format to all three changed modules; fix invalid LaTeX escape sequence in density ylabel.
Cover _bin_pore radial bins, init_diffusion_vacf guard conditions (non-trr rejection, wrong direction, mode conflict), and _diffusion_vacf_data output shape.
- Add _atoms_per_mol to __init__ (total atoms per mol in trajectory) - Fix _numpy() and VACF _sample_helper block to use .reshape(n_res, _atoms_per_mol, 3)[:, self._atoms, :] so partial atom selections (e.g. atoms=["C1"]) compute correct CoM instead of raising a reshape error - Fix self._masses → self._masses_arr (ndarray) in _numpy() - Update benchmark table to Python 3.13 / 16-core measurements - Add VACF new-feature section to changelog
- sample.py: add self.num_res = n_res in __init__ so VACF and numpy sampling paths can reference the residue count without AttributeError - sample.py: _diffusion_vacf wall mask now uses true radial distance from the pore axis for all binning directions; previously it used the binning axis coordinate, which is wrong for axial (z) sampling in pore systems - diffusion.py: integrate_bin_diffusion_vacf guards against zero-density bins with np.where; previously divided by zero, producing silent NaN in all downstream VACF integrals for unoccupied bins - mc.py: parallel MC assembly loop now copies fluc_diff_bin and fluc_df_bin from each worker; previously only worker-0's values were kept - tests: add test_vacf_zero_density (unit) and test_diffusion_mc_parallel_keys (integration) to cover the fixed paths; extend test_vacf_init to assert num_res is set and consistent with the residue map
- sample.py: assign self.num_res in __init__ (AttributeError on all VACF paths) - sample.py: fix in_wall_mask to use true radial distance for non-radial pore directions - diffusion.py: prevent NaN in zero-density VACF bins; use np.where guard - mc.py: assemble fluc_diff_bin/fluc_df_bin from all parallel workers, not just worker 0 - diffusion.py: remove is_error parameter from mc_profile (dead code caused KeyError) - Remove dead functions: cui, column, num_dens_to_mass_dens, mc_statistics, mc_lag_time - Restore commented-out radial MC stubs (log_likelihood_radial, setup_bessel_box, mc_fit_radial, mc_profile_radial) for future implementation - Replace all non-ASCII chars (em dashes, arrows, special symbols) with ASCII equivalents - Convert docs/diffusion_vacf.rst to MyST Markdown - Add tests: test_vacf_init num_res check, test_vacf_zero_density, test_diffusion_mc_parallel_keys - Update benchmark numbers to median of 3 runs - tables.py: remove unused numpy import (ruff F401)
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
2 participants
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.
v1.0.0
Performance
Benchmarked on an Apple Silicon macOS machine (2001-frame cylinder trajectory, benzene,
13 lag times). Timings are the mean of 3 consecutive runs.
Direct comparison against PyPI v0.2.3 is blocked by its CMake/scikit-build dependency,
so microbenchmarks on the core hot paths are provided instead.
Parallel sampling is 5–7× faster than the serial equivalent on all platforms.
Earlier builds measured parallel density at 1.33 s and parallel MC at 1.59 s on
macOS because Python 3.12 changed the default multiprocessing start method from
forktospawn, causing each worker to re-import all heavy dependencies onevery call. The explicit
mp.get_context("fork")fix restores expected speedup(~13× improvement in parallel wall time on macOS versus the spawn baseline).
Hot-path microbenchmarks (2001 frames, isolated from I/O):
Key changes driving the speedup:
sample.py—_sample_helperhot loop: position extraction vectorised aspositions[atom_indices] / 10.0 + shift_arr; COM calculated viamasses_arr @ pos / sum_masses(NumPy dot product replaces per-atom Python loop);_masses_arrpre-computed once in__init__as a float64 NumPy array; residue atom index arrays pre-built asnp.array([...])at init timesample.py— parallel diffusion-bin merge: replaced O(bin_num × len_window) triple-nested Python loop with(np.array(a) + np.array(b)).tolist()per keysample.py/mc.py— explicitmp.get_context("fork")on non-Windows platforms; prevents Python 3.12 macOSspawndefault from re-importing dependencies per worker (~13× parallel speedup on macOS)density.py— bin volume calculation vectorised:np.pi * plen * (w[1:]**2 - w[:-1]**2)replaces scalar loop; weighted-mean integration usesnp.dot+ boolean maskdiffusion.py— MSD normalisation and bin slope calculation fully vectorised with NumPy arrays; Bessel function series usesnp.expon a coefficient array; removed unusedmathanditertoolsimportsangle.py/gyration.py— density-weighted normalisation usesnp.where(dens != 0, val / dens, 0.0); mean computed withnp.mean; mean line rendered vianp.full_likeadsorption.py—np.sumreplaces accumulator loops for reservoir and pore molecule countsmc.py—n_procparameter no longer shadows thenp(NumPy) import; duplicateimport numpy as numpyremoved; dead commented-out code removed from__init__Logic fixes
mc.py—list_diff_coeffwas incorrectly assignedlist_diff_profile(per-bin profile data) instead of the actual per-step model coefficients; silently returned wrong values for all MC diffusion runsmc.py/sample.py— function parameternp=0shadowed thenumpyalias, causingAttributeError: 'int' has no attribute 'array'in all parallel and post-merge code paths; renamed ton_procdiffusion.py— residualmath.floor/math.ceilcalls remained afterimport mathwas removed; converted toint(np.floor(...))/int(np.ceil(...))adsorption.py— reservoir maskex_width[:-1](150 elements) applied toex_bins(151 elements) causedIndexError; mask now usesex_widthdirectlyutils.py/tables.py—file_to_textandmc_resultsstill accessed the old flat pore structure (pore["box"],pore["diam"],pore["res"],pore["type"]) after the data model was updated topore["box"]["dimensions"],pore["box"]["res"],pore[shape_id]["diam"]; fixed across all four branches (gyr_bin,diff_bin,dens_bin,mc) and intables.pyutils.py—file_to_textMC branch usedfree_energy[0][i][1:](99 elements) in a DataFrame alongside 100-element arrays;[1:]removedmodel.py— debugprintcalls for_d0and_diff_binremovedmodel.py— duplicateself._sys_props = {}assignment removedTests
tests/test_simple.pyfrom unittest to pytesttest_unit.py(fast, no trajectory) andtest_integration.py(full pipeline)conftest.pyfixture runs all trajectory sampling once per session; pre-computes MC output forfile_to_texttestsdensity.mean()return values;adsorption.calculate()output structure;density/gyration/angle.bins_plot()intent "in"/"ex" and normalised x-axis;diffusion.bins()output structure; MC output key correctness (list_diff_coeffvslist_diff_profile);utils.file_to_text()for all four output types (dens_bin pore, dens_bin box, diff_bin, gyr_bin, mc); YAML round-trip intest_utilsbench_compare.pyadded: standalone script for sampling speed comparison (python tests/bench_compare.py)Documentation
docsrc/todocs/; previous built HTML preserved atdocs/v_old/CI / tooling
lint.yml)security.yml)python_requiresbumped to>=3.12setup.py+MANIFEST.into a singlepyproject.toml(PEP 517/621);[tool.ruff]config addedpiptouv(astral-sh/setup-uv@v5);requirements.txtremoved (deps resolved viapyproject.toml)codeql.yml: updated actions from@v2to@v3; removed irrelevantjavascriptlanguage scanAdministrative
pyproject.toml: version 1.0.0,requires-python = ">=3.12", author email updated, chemfiles unpinned, porems dependency bumped to>=1.0.0;[project.optional-dependencies]dev group added (pytest,pytest-cov)