PyPI readiness: Finalization of patchsim#3
Merged
snehasaisneha merged 55 commits intodevfrom Apr 7, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub Actions CI and PyPI publish workflows, pre-commit config, packaging metadata, extensive docs and templates, a public SDK entrypoint and subcommand CLI, refactors simulation core for per‑patch parameters and evaluated transition expressions with a Model wrapper, updates visualization/logging, and adds a broad pytest suite. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant User
participant CLI_SDK
end
rect rgba(200,255,200,0.5)
participant Simulation
participant Model
end
rect rgba(255,200,200,0.5)
participant ODE_Solver
participant Visualization
end
User->>CLI_SDK: run command / load_config()
CLI_SDK->>Simulation: setup_simulation(config)
Simulation->>Simulation: parse Transitions, build patches and parameters
Simulation->>Model: instantiate Model(network_model, compartments)
Model->>Model: construct_ode() (build rhs using per-patch FOI)
CLI_SDK->>Model: solve(y0, t_range)
Model->>ODE_Solver: odeint(rhs, y0, t_range)
ODE_Solver-->>Model: solution arrays
Model->>Visualization: visualize(results, patches, patch_parameters)
Visualization-->>User: write plots / outputs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/patchsim/core/model.py (1)
118-132:⚠️ Potential issue | 🟠 MajorThese derivatives are still patch-local, so
network_matrixhas no effect.At this point the method only applies rates computed from each patch’s local state; neither
self.networknorcompute_force_of_infection()feeds into those rates.simulate_ode()andsimulate_discrete()therefore produce the same trajectories for connected and disconnected multi-patch models, so cross-patch transmission is missing from this public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/core/model.py` around lines 118 - 132, The derivatives are computed only from local rates so cross-patch transmission is ignored; update the loop that applies rates (the block using rates = self.base_model.compute_rates(patch_state) and iterating self.base_model.transitions) to incorporate network-mediated force of infection by calling or computing per-patch force via compute_force_of_infection (and/or using self.network/network_matrix) and adjusting any infection-type transition rates before updating derivatives. Concretely: for each patch i, compute foI_i = self.compute_force_of_infection(i, full_state_or_patch_states) (or derive it from self.network), then modify the relevant transition rate(s) in rates[transition_label] (e.g., multiply or replace infection transition rates) prior to applying the decrement/increment to derivatives[f"{source}_{i}"] and derivatives[f"{target}_{i}"]; keep restoring self.base_model.parameters (old_params) as before. Ensure compute_force_of_infection and self.network are fed the appropriate state vector so simulate_ode()/simulate_discrete() reflect connected dynamics.
🧹 Nitpick comments (5)
test.py (1)
1-4: Consider removing this unused file.An empty deprecated file at the repository root serves no purpose and may confuse developers or tooling. If it's genuinely required for compatibility (as the docstring claims), please clarify what requires it. Otherwise, remove this file entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test.py` around lines 1 - 4, test.py is an empty deprecated compatibility placeholder that should either be removed or clarified; if it's not required, delete test.py to avoid confusion, and remove any references to it in packaging/CI; if it must remain, replace the current vague docstring with a clear statement naming what requires the file (e.g., "Required by X tooling/version Y for compatibility"), add a TODO with a removal timeframe, and optionally add a unit test or comment linking to the external requirement so future maintainers can understand why test.py exists.docs/getting-started.md (1)
5-13: Consider adding PyPI installation instructions.Since the PR prepares patchsim for PyPI publication, the "Getting Started" page should include PyPI installation as the primary method:
pip install patchsimThe current git clone + editable install is more appropriate for contributors/developers. Consider restructuring this section to show PyPI installation first, with development installation in a separate "For Contributors" or "Development Setup" subsection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started.md` around lines 5 - 13, Update the "Installation" section to present PyPI as the primary install method by placing a single-line instruction "pip install patchsim" first under the "Installation" heading, then move the current git clone + pip install -e . block into a new subsection titled "For Contributors" or "Development Setup" (or similar) and keep the editable install instructions there; ensure headings "Installation" and the new contributor subsection are updated accordingly so readers see PyPI install first and development install separately.docs/mathematical-model.md (1)
10-15: Add one explicit index-mapping sentence forW_ij.To avoid source/target confusion, explicitly state that
jis source andiis target (matchingsource,targetCSV). This reduces implementation ambiguity when building the matrix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/mathematical-model.md` around lines 10 - 15, Clarify the index mapping for W_ij by adding a sentence stating that in the weight matrix W, the first index i is the target (receiving) patch and the second index j is the source (originating) patch, so W_ij denotes the weight from source j to target i; reference this explicitly alongside the force-of-infection formula λ_i(t) = Σ_j W_ij * (I_j / N_j) to match the source, target ordering used in the CSV and avoid confusion between I_j and N_j as source quantities.configs/sample-sir-ode.yaml (1)
22-22: Block-styleTransitionswith quoted keys improves consistency and readability.Current inline flow mapping parses correctly but block-style with quoted keys is a better convention for complex mappings with special characters. This improves clarity for developers and ensures compatibility across different YAML consumers.
♻️ Proposed refactor
-Transitions: {S -> I: "beta", I -> R: "gamma * I"} +Transitions: + "S -> I": "beta" + "I -> R": "gamma * I"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/sample-sir-ode.yaml` at line 22, The Transitions mapping is currently an inline flow mapping which is harder to read and may break with special characters; change the inline "Transitions: {S -> I: \"beta\", I -> R: \"gamma * I\"}" to a block-style mapping with quoted keys and values (e.g., use a top-level Transitions: followed by indented lines quoting "S -> I": "beta" and "I -> R": "gamma * I") so the mapping keys like "S -> I" and "I -> R" and their expressions "beta" and "gamma * I" are clear and YAML-safe..github/workflows/ci.yml (1)
5-5: Align push triggers with active default integration branch.Given this PR targets
dev, consider addingdevto the push trigger list (or replacingdevelop) so direct pushes to the integration branch always run CI.♻️ Proposed tweak
- branches: [main, feature, develop] + branches: [main, feature, dev, develop]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 5, Update the workflow push trigger branches array so it includes the active integration branch `dev` instead of or in addition to `develop`; locate the `branches: [main, feature, develop]` entry in the CI YAML and modify it to include `dev` (e.g., add `dev` to the list or replace `develop`), ensuring pushes to the current integration branch will run CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 1-15: Update the pinned revs for the two repos: change the
pre-commit-hooks rev from v5.0.0 to v6.0.0 and the ruff-pre-commit rev from
v0.11.13 to v0.15.8 by editing the rev values for repo:
https://github.com/pre-commit/pre-commit-hooks and repo:
https://github.com/astral-sh/ruff-pre-commit in .pre-commit-config.yaml; before
committing, confirm your project Python requirement (>=3.11) satisfies
pre-commit-hooks v6.0.0, ensure you are not relying on removed hooks
(check-byte-order-marker, fix-encoding-pragma) since only ids present are
end-of-file-fixer, trailing-whitespace, check-yaml, check-toml and
ruff/ruff-format, then run pre-commit autoupdate or run pre-commit hooks
(pre-commit run --all-files) and fix any issues reported.
In `@docs/configuration.md`:
- Around line 5-23: The example YAML under "Model definition" is missing
required runtime input-file keys and will not run as-is; update the snippet to
include the input file keys (PatchFile, NetworkFile, SeedFile) before the
ModelName or explicitly mark the block as a partial example. Specifically add
entries like PatchFile, NetworkFile, and SeedFile (referenced by those exact
keys) above or alongside ModelName in the example so users can copy-paste a
runnable config, or add a short comment/header saying "Partial example — input
files omitted" if you don't want to include real paths.
In `@docs/index.md`:
- Line 15: Fix the two typos in the feature bullets: change "models that
supports" to "models that support" in the Flexible Compartment Models bullet
(the phrase containing "SIR, SEIR") and correct the misspelling "Extensiblity"
to "Extensibility" in the Extensibility/Extensiblity bullet so both grammar and
spelling are accurate.
In `@docs/network-design.md`:
- Around line 18-19: Clarify that normalization of outgoing weights is performed
per day and per source patch: update the sentence that currently reads "Rows are
normalized so that outgoing weights from each patch sum to 1." to explicitly
state normalization is done "for each day and for each source patch (i.e.,
normalize outgoing weights per day, per source patch)" so users with a day
column won't mistakenly normalize across all days and distort force-of-infection
calculations.
In `@docs/reference/models.md`:
- Around line 50-54: Update the docs to reference the installed CLI command
instead of the module name: replace the "Run via `patchsim.cli`" instruction
with a concrete example using the exposed CLI (e.g., "Run via `patchsim run -c
<model_script>`" or another appropriate subcommand), ensuring you mention the
`patchsim` command and the `run -c` option so readers know how to invoke the new
CLI interface.
In `@mkdocs.yml`:
- Around line 41-44: The mkdocstrings Python handler path is incorrectly pointed
at ["src/patchsim"], causing imports like patchsim.utils.loader and
patchsim.core.model to be resolved to non-existent nested directories; update
the handlers.python.paths setting in mkdocs.yml to ["src"] so mkdocstrings can
find the top-level package patchsim at src/patchsim and correctly resolve its
submodules.
In `@pyproject.toml`:
- Around line 87-92: The sdist include list under
[tool.hatch.build.targets.sdist] currently restricts files to the listed
patterns and omits the package source; update the include array to add the
package directory pattern (e.g. add "/src/patchsim/**") so the importable code
is packaged in the source distribution and source installs won't be broken,
making the change in the include list in the same section of pyproject.toml.
In `@src/patchsim/cli.py`:
- Around line 53-58: The current logic may rmtree the current working directory
or try to remove a file; before calling shutil.rmtree(project_dir) add guards:
ensure project_dir.exists() and project_dir.is_dir() and project_dir.resolve()
!= Path.cwd().resolve() and that the caller explicitly passed force; if
project_dir exists but is a file raise a clear FileExistsError, and if
project_dir.resolve() == Path.cwd().resolve() raise a ValueError (or similar)
refusing to operate on '.'/CWD even with --force; only call
shutil.rmtree(project_dir) when all these checks pass (references: project_dir,
force, Path(name), shutil.rmtree).
In `@src/patchsim/core/model_runner.py`:
- Around line 47-50: In solve(self, y0, t_range) the list comprehension y0_vec =
[y0[v] for v in self.all_vars] can raise a raw KeyError; first validate that all
variables in self.all_vars are present in the y0 mapping by computing missing =
[v for v in self.all_vars if v not in y0] and if missing is non-empty raise a
ValueError listing the missing variable names (e.g. "Missing initial values for:
..."); then proceed to build y0_vec and call odeint as before.
- Around line 33-37: The FOI scaling is only applied when src == "S" and tgt ==
"I", which misses other infection transitions (e.g., S -> E) — change the
condition so FOI multiplies any susceptible-to-infection transition rather than
only S->I: detect infection transitions by checking tgt against the set of
infection compartments (e.g., {"E","I"} or a model-provided infection target
list) or by using a transition metadata flag if available, then compute
adjusted_rate = beta * state[f"S_{i}"] * lambdas[i] for those transitions (use
the existing variables src, tgt, beta, state[f"S_{i}"], lambdas to locate and
update the logic).
In `@src/patchsim/core/model.py`:
- Around line 105-110: The code currently selects patch parameters using
patch_names = list(self.patch_parameters.keys()) and index i, which relies on
dict insertion order and can misalign with PatchFile ordering; change the lookup
to use the canonical ordering provided by self.patch_names (plumbed from
setup_simulation()) so patch_name = self.patch_names[i] (with a safe fallback if
self.patch_names is missing or i out of range) and then merge parameters via
patch_params = {**self.base_model.parameters,
**self.patch_parameters.get(patch_name, {})}; update references in the function
that uses patch_parameters/patch_name to rely on self.patch_names instead of
list(self.patch_parameters.keys()) to preserve correct patch-to-parameter
mapping.
- Around line 29-35: The current string-substitution-and-eval flow in the branch
handling rate_expr (when isinstance(rate_expr, str)) is unsafe and brittle;
replace it by parsing/evaluating the expression with a safe evaluator that
receives a bindings dict built from self.parameters and state (e.g., variables =
{**self.parameters, **state}) instead of doing text replacements. Concretely, in
the block around rate_expr / rate_val, remove the for-loop replacements and
eval(), and call a safe evaluator such as sympy.sympify(expression,
locals=variables) or an asteval/numexpr evaluator, ensuring no access to Python
builtins/globals, catching and logging parse/eval errors, and assigning the
numeric result to rate_val; keep references to the same symbols (rate_expr,
self.parameters, state, rate_val) so the change is localized.
In `@src/patchsim/core/simulation.py`:
- Around line 59-63: The code forces num_patches = 1 when NetworkFile is
missing, which breaks shape consistency with patches and y0; instead, derive
num_patches from the built patches (or the rows in PatchFile) and construct
network_matrix to match that size (e.g., an all-zero or identity NxN matrix
where N = len(patches)), so NetworkModel and initial state y0 have consistent
dimensions; update the branch that checks "NetworkFile" to set num_patches =
len(patches) (or equivalent) and build network_matrix of shape (num_patches,
num_patches).
In `@src/patchsim/utils/logger.py`:
- Around line 36-41: The current loop over config["PatchParameters"] can crash
if items are not dicts or missing "patch"; update the logging in the block that
checks "PatchParameters" to defensively handle malformed entries: ensure each
entry is a dict (use isinstance check on entry), retrieve patch via
entry.get("patch") and parameters via entry.get("parameters", {}) rather than
indexing, skip or continue when patch is None or invalid, and catch unexpected
exceptions around the per-entry logging (log a warning via logger.warning
including the problematic entry) so setup does not raise from logger.info for
malformed PatchParameters.
In `@src/patchsim/utils/viz.py`:
- Around line 11-14: Add an early guard in the function using the patches list
so empty input won't compute ncols==0: check if not patches and
return/early-exit (or create an empty figure) before computing n, ncols, nrows
and calling plt.subplots; then replace any cleanup logic that relies on a loop
index i with index-independent iteration over axes (e.g., iterate axes.flat or
use enumerate over axes.flatten()) so removal/clearing of unused axes doesn't
reference a non-existent i; update references to n, ncols, nrows, fig, and axes
accordingly.
In `@tests/test_smoke.py`:
- Around line 72-76: The test currently calls
load_config("configs/sample-sir-ode.yaml") which assumes the working directory;
update test_yaml_model_loading to resolve the sample config path relative to the
test file before calling load_config (use the test module's file location via
__file__ and Path/Pathlib to build an absolute path to
"configs/sample-sir-ode.yaml"), then pass that resolved path to load_config so
the test works regardless of the current working directory; locate this change
in the test function test_yaml_model_loading that imports load_config from
patchsim.core.simulation.
---
Outside diff comments:
In `@src/patchsim/core/model.py`:
- Around line 118-132: The derivatives are computed only from local rates so
cross-patch transmission is ignored; update the loop that applies rates (the
block using rates = self.base_model.compute_rates(patch_state) and iterating
self.base_model.transitions) to incorporate network-mediated force of infection
by calling or computing per-patch force via compute_force_of_infection (and/or
using self.network/network_matrix) and adjusting any infection-type transition
rates before updating derivatives. Concretely: for each patch i, compute foI_i =
self.compute_force_of_infection(i, full_state_or_patch_states) (or derive it
from self.network), then modify the relevant transition rate(s) in
rates[transition_label] (e.g., multiply or replace infection transition rates)
prior to applying the decrement/increment to derivatives[f"{source}_{i}"] and
derivatives[f"{target}_{i}"]; keep restoring self.base_model.parameters
(old_params) as before. Ensure compute_force_of_infection and self.network are
fed the appropriate state vector so simulate_ode()/simulate_discrete() reflect
connected dynamics.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 5: Update the workflow push trigger branches array so it includes the
active integration branch `dev` instead of or in addition to `develop`; locate
the `branches: [main, feature, develop]` entry in the CI YAML and modify it to
include `dev` (e.g., add `dev` to the list or replace `develop`), ensuring
pushes to the current integration branch will run CI.
In `@configs/sample-sir-ode.yaml`:
- Line 22: The Transitions mapping is currently an inline flow mapping which is
harder to read and may break with special characters; change the inline
"Transitions: {S -> I: \"beta\", I -> R: \"gamma * I\"}" to a block-style
mapping with quoted keys and values (e.g., use a top-level Transitions: followed
by indented lines quoting "S -> I": "beta" and "I -> R": "gamma * I") so the
mapping keys like "S -> I" and "I -> R" and their expressions "beta" and "gamma
* I" are clear and YAML-safe.
In `@docs/getting-started.md`:
- Around line 5-13: Update the "Installation" section to present PyPI as the
primary install method by placing a single-line instruction "pip install
patchsim" first under the "Installation" heading, then move the current git
clone + pip install -e . block into a new subsection titled "For Contributors"
or "Development Setup" (or similar) and keep the editable install instructions
there; ensure headings "Installation" and the new contributor subsection are
updated accordingly so readers see PyPI install first and development install
separately.
In `@docs/mathematical-model.md`:
- Around line 10-15: Clarify the index mapping for W_ij by adding a sentence
stating that in the weight matrix W, the first index i is the target (receiving)
patch and the second index j is the source (originating) patch, so W_ij denotes
the weight from source j to target i; reference this explicitly alongside the
force-of-infection formula λ_i(t) = Σ_j W_ij * (I_j / N_j) to match the source,
target ordering used in the CSV and avoid confusion between I_j and N_j as
source quantities.
In `@test.py`:
- Around line 1-4: test.py is an empty deprecated compatibility placeholder that
should either be removed or clarified; if it's not required, delete test.py to
avoid confusion, and remove any references to it in packaging/CI; if it must
remain, replace the current vague docstring with a clear statement naming what
requires the file (e.g., "Required by X tooling/version Y for compatibility"),
add a TODO with a removal timeframe, and optionally add a unit test or comment
linking to the external requirement so future maintainers can understand why
test.py exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ed6398a-10b4-4b5b-8199-883f0cb2ff09
⛔ Files ignored due to path filters (7)
data/networks/sample-network-static.csvis excluded by!**/*.csvdata/patch/sample-sir-ode-patch-population.csvis excluded by!**/*.csvdata/seeds/sample-sir-ode-patchA-2.csvis excluded by!**/*.csvsrc/patchsim/templates/project/data/networks/network-static.csvis excluded by!**/*.csvsrc/patchsim/templates/project/data/patch/patch-population.csvis excluded by!**/*.csvsrc/patchsim/templates/project/data/seeds/seed-initial.csvis excluded by!**/*.csvuv.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.github/workflows/ci.yml.github/workflows/publish.yml.gitignore.pre-commit-config.yamlCHANGELOG.mdREADME.mdconfigs/sample-sir-ode.yamldocs/architecture.mddocs/configuration.mddocs/getting-started.mddocs/index.mddocs/mathematical-model.mddocs/network-design.mddocs/reference/models.mddocs/reference/utils.mddocs/results.mddocs/simulation-workflow.mdmkdocs.ymlpyproject.tomlsrc/patchsim/__init__.pysrc/patchsim/cli.pysrc/patchsim/core/model.pysrc/patchsim/core/model_runner.pysrc/patchsim/core/simulation.pysrc/patchsim/models/ka_fmd_sirsv_discrete.pysrc/patchsim/templates/project/config.yamlsrc/patchsim/templates/project/output/.gitkeepsrc/patchsim/utils/loader.pysrc/patchsim/utils/logger.pysrc/patchsim/utils/viz.pytest.pytests/conftest.pytests/test_config.pytests/test_model.pytests/test_simulation_setup.pytests/test_smoke.py
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/patchsim/core/simulation.py (1)
49-55:⚠️ Potential issue | 🟡 MinorUnknown seed patches should raise a clear validation error.
populations[patch]currently throws a rawKeyErrorwhen a seed row references a patch missing fromPatchFile. Make this an explicit config validation error.♻️ Suggested fix
for _, row in seed_df.iterrows(): patch = row["patch"] + if patch not in populations: + raise ValueError(f"SeedFile contains unknown patch '{patch}' not present in PatchFile") total = sum(row[c] for c in compartments) if not all(row[c] >= 0 for c in compartments): raise ValueError(f"Seed values must be non-negative for patch {patch}") if abs(total - populations[patch]) >= EPSILON: raise ValueError(f"Seed values do not sum to population for patch {patch}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/core/simulation.py` around lines 49 - 55, The loop that reads seed rows uses populations[patch] which raises a raw KeyError for unknown patch names; instead, explicitly validate the patch exists before using it and raise a clear config validation error. In the seed_df.iterrows() loop, check "if patch not in populations:" and raise a descriptive exception (e.g., ValueError or the project’s ConfigValidationError if one exists) like "Unknown seed patch '{patch}' not listed in PatchFile" before computing total or comparing to populations[patch]; keep the existing non-negative and sum checks (using EPSILON) after this validation.
♻️ Duplicate comments (1)
src/patchsim/cli.py (1)
55-66:⚠️ Potential issue | 🔴 CriticalBroaden the
--forceguard to block root and parent paths.Line 59 only rejects the exact current working directory.
patchsim init .. --forcefrom a subdirectory, orpatchsim init / --force, still falls through toshutil.rmtree(project_dir)and can wipe the active tree or filesystem root.🛡️ Suggested guardrail
def _cmd_init(name: str, force: bool = False) -> None: project_dir = Path(name) # Safety checks: prevent deleting cwd or non-directories resolved = project_dir.resolve() + cwd = Path.cwd().resolve() if project_dir.exists() and not project_dir.is_dir(): raise NotADirectoryError(f"Target exists and is not a directory: {project_dir}") - if force and resolved == Path.cwd().resolve(): - raise ValueError(f"Refusing to overwrite current working directory: {resolved}") + if force and (resolved == Path(resolved.anchor) or resolved == cwd or resolved in cwd.parents): + raise ValueError(f"Refusing to overwrite unsafe target: {resolved}") if project_dir.exists() and any(project_dir.iterdir()) and not force: raise FileExistsError(f"Refusing to overwrite existing directory: {project_dir}. Use --force to overwrite.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/cli.py` around lines 55 - 66, The current force-guard only rejects exact matches with Path.cwd(); update the check around project_dir/resolved/force before calling shutil.rmtree to also block parents and root: if resolved is the current cwd or is an ancestor of the current cwd (e.g. Path.cwd().resolve().relative_to(resolved) succeeds) or resolved is the filesystem root/anchor, raise a ValueError (or similar) instead of allowing shutil.rmtree to run; reference the variables and helpers project_dir, resolved, Path.cwd(), force, and shutil.rmtree so you locate and replace the existing guard logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patchsim/cli.py`:
- Around line 68-73: The package is missing non-Python template files in the
wheel so runtime calls to resources.files("patchsim").joinpath("templates",
"project") (used by template_root and _copy_template_tree when initializing
project_dir and writing config.yaml) fail; modify the build config under
[tool.hatch.build.targets.wheel] to explicitly include the templates directory
(e.g., add an include/artifacts entry that bundles patchsim/templates/** or the
templates/project tree) so that resources.files can find the files at runtime
after installation.
In `@src/patchsim/core/model.py`:
- Around line 101-107: The two conditional branches around computing lambdas are
redundant because both call self.compute_force_of_infection(state); simplify by
removing the if/else and directly assign lambdas =
self.compute_force_of_infection(state), eliminating the unused conditional
(references: variable lambdas and method compute_force_of_infection, and
attributes self.num_patches/self.network).
- Around line 127-131: The code temporarily mutates shared state by assigning
patch-specific values to self.base_model.parameters before calling
compute_rates, which can race with concurrent calls; change compute_rates on the
base model to accept an optional parameters override (e.g., def
compute_rates(state, parameters=None)) and update compute_derivatives to call
self.base_model.compute_rates(patch_state, parameters=patch_params) instead of
mutating self.base_model.parameters; remove the old temporary assignment/restore
and update any other callers of compute_rates to preserve current behavior when
no override is provided.
- Around line 145-157: The check `"beta" in str(rate)` is wrong because rate is
a float; instead inspect the original transition rate expression from the
transition definitions and test that for the substring "beta". Replace the
condition with a lookup into the transition definition (e.g., use the original
expression stored under a variable like transitions[transition_label],
transition_defs[transition_label], or
model.transitions[transition_label]["rate"] depending on the codebase) and do
`"beta" in str(original_rate_expr)`; keep the rest of the logic updating
adjusted_rate (using patch_params, patch_state, lambdas, etc.) the same.
- Around line 115-125: The branch checking self.patch_names is dead because
patch_names is never set; either initialize patch_names where patch_parameters
is configured or derive a stable ordering elsewhere. Update setup_simulation in
simulation.py to set net.patch_names (e.g., list of patch keys in the canonical
order) alongside net.patch_parameters so the branch in model.py (referencing
self.patch_names and self.patch_parameters) becomes reachable and uses the
stable ordering, or alternatively modify model.py to obtain ordering from the
same canonical source that setup_simulation uses instead of relying on dict key
order.
In `@src/patchsim/core/simulation.py`:
- Around line 140-142: The code silently truncates config["TMax"] via int(...)
when constructing t_range and can allow non-positive or fractional horizons;
validate config["TMax"] before building t_range: check that config["TMax"] is
numeric and > 0 and either an integer or explicitly convert with a documented
rule (e.g., use math.ceil if you want to round up), and raise a ValueError with
a clear message if validation fails; then compute the number of steps (use a
validated n_steps variable instead of raw int(config["TMax"])) and build t_range
and call Model/net and model.solve as before (referencing config["TMax"],
t_range, Model, and model.solve to locate the change).
- Around line 93-104: The code currently assigns entry["parameters"] directly to
patch_params[patch_name], dropping global defaults; change the logic that builds
patch_params from config["PatchParameters"] so each entry's parameters are
merged on top of global_params (e.g., patch_params[patch_name] =
{**global_params, **entry.get("parameters", {})}) to preserve global keys, and
add a validation step after parsing config["PatchParameters"] to warn or error
if a configured patch_name is not present in patches (to surface typos) rather
than silently ignoring them.
---
Outside diff comments:
In `@src/patchsim/core/simulation.py`:
- Around line 49-55: The loop that reads seed rows uses populations[patch] which
raises a raw KeyError for unknown patch names; instead, explicitly validate the
patch exists before using it and raise a clear config validation error. In the
seed_df.iterrows() loop, check "if patch not in populations:" and raise a
descriptive exception (e.g., ValueError or the project’s ConfigValidationError
if one exists) like "Unknown seed patch '{patch}' not listed in PatchFile"
before computing total or comparing to populations[patch]; keep the existing
non-negative and sum checks (using EPSILON) after this validation.
---
Duplicate comments:
In `@src/patchsim/cli.py`:
- Around line 55-66: The current force-guard only rejects exact matches with
Path.cwd(); update the check around project_dir/resolved/force before calling
shutil.rmtree to also block parents and root: if resolved is the current cwd or
is an ancestor of the current cwd (e.g.
Path.cwd().resolve().relative_to(resolved) succeeds) or resolved is the
filesystem root/anchor, raise a ValueError (or similar) instead of allowing
shutil.rmtree to run; reference the variables and helpers project_dir, resolved,
Path.cwd(), force, and shutil.rmtree so you locate and replace the existing
guard logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 128b0c0f-575f-44d1-b7d8-c8d567b520a2
📒 Files selected for processing (19)
.github/workflows/ci.yml.pre-commit-config.yamlconfigs/sample-sir-ode.yamldocs/configuration.mddocs/getting-started.mddocs/index.mddocs/mathematical-model.mddocs/network-design.mddocs/reference/models.mdmkdocs.ymlpyproject.tomlsrc/patchsim/cli.pysrc/patchsim/core/model.pysrc/patchsim/core/model_runner.pysrc/patchsim/core/simulation.pysrc/patchsim/utils/logger.pysrc/patchsim/utils/viz.pytest.pytests/test_smoke.py
✅ Files skipped from review due to trivial changes (10)
- .pre-commit-config.yaml
- docs/getting-started.md
- docs/network-design.md
- test.py
- docs/index.md
- docs/configuration.md
- .github/workflows/ci.yml
- docs/mathematical-model.md
- tests/test_smoke.py
- src/patchsim/core/model_runner.py
🚧 Files skipped from review as they are similar to previous changes (5)
- configs/sample-sir-ode.yaml
- src/patchsim/utils/viz.py
- docs/reference/models.md
- src/patchsim/utils/logger.py
- pyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/patchsim/core/model.py (1)
115-121:⚠️ Potential issue | 🟠 MajorDict key ordering still unreliable for patch parameter lookup.
The code uses
list(self.patch_parameters.keys())[i]to match patches by index, which depends on dict insertion order. While Python 3.7+ guarantees insertion order, this assumespatch_parameterskeys were inserted in the same order as patches inPatchFile. If the YAMLPatchParameterssection lists patches in a different order, parameters will be silently misapplied.Per the relevant code snippet from
simulation.py:93-107,patch_paramsis built by iteratingconfig["PatchParameters"](user-defined order), then augmented for remaining patches. This order may not match thepatcheslist fromPatchFile.🐛 Proposed fix using patch index directly
if hasattr(self, "patch_parameters"): - patch_names = list(self.patch_parameters.keys()) - patch_name = patch_names[i] if i < len(patch_names) else None - patch_params = {**self.base_model.parameters, **self.patch_parameters.get(patch_name, {})} + # Use patch index to derive canonical patch name from patch_parameters + # Requires simulation.py to attach patch_names alongside patch_parameters + if hasattr(self, "patch_names") and i < len(self.patch_names): + patch_name = self.patch_names[i] + else: + patch_name = list(self.patch_parameters.keys())[i] if i < len(self.patch_parameters) else None + patch_params = {**self.base_model.parameters, **self.patch_parameters.get(patch_name, {})} else: patch_params = self.base_model.parametersA cleaner solution: have
simulation.pyalso attachnet.patch_names = patchesso the canonical order is always available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/core/model.py` around lines 115 - 121, The patch parameter lookup currently uses list(self.patch_parameters.keys()) which can mismatch the canonical patch order; update the codeflow so simulation.py sets the canonical order by attaching the patches list to the network (e.g., net.patch_names = patches), then change model.py to use that canonical list (self.patch_names) when resolving patch_params (fall back to base_model.parameters or self.patch_parameters.get(patch_name) as before) instead of relying on dict key order; reference the symbols patch_parameters, base_model.parameters, PatchFile/paches list, and add/consume net.patch_names to ensure correct mapping.
🧹 Nitpick comments (3)
src/patchsim/core/simulation.py (2)
163-164: Remove commented-out code.Line 163 contains a commented-out call to
plot_patch_subplots. Sincemodel.visualize()now handles plotting, this dead code should be removed.♻️ Proposed cleanup
- # plot_patch_subplots(t_range, out_ode, patches, plots_dir, model_name) model.visualize(t_range, out_ode, patches, plots_dir, model_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/core/simulation.py` around lines 163 - 164, Remove the dead commented-out call to plot_patch_subplots: delete the commented line containing "plot_patch_subplots(t_range, out_ode, patches, plots_dir, model_name)" so only the active model.visualize(t_range, out_ode, patches, plots_dir, model_name) call remains; this cleans up the code now that model.visualize handles plotting and avoids confusion about duplicate plotting functions.
25-29: Consider addingTransitionsandTMaxto required fields.
TransitionsandTMaxare validated later with specific error messages, but including them inrequired_fieldsprovides early, consistent feedback. Currently, missingTransitionstriggers line 84's error, and missingTMaxtriggers line 146's error—both work, but the error messages differ in style from the "Missing required field" pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/core/simulation.py` around lines 25 - 29, The required_fields list currently contains ["PatchFile", "SeedFile", "OutputDir"] but omits "Transitions" and "TMax", causing inconsistent error styles; update the required_fields variable to include "Transitions" and "TMax" so missing config keys raise the same "Missing required field" ValueError; after adding them, ensure the later specific checks for Transitions and TMax (the validation logic referencing those keys) either remain as more detailed validation (not duplicate the missing-key error) or are adjusted to assume presence and only validate content.src/patchsim/core/model.py (1)
134-135: Hardcoded infection compartments limit model flexibility.The set
{"I", "E"}assumes specific compartment naming conventions. Models using different naming (e.g.,"Exposed","Infected","I1","I2"for staged infections) won't have network FOI applied correctly.Consider making this configurable via the model or transition metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/core/model.py` around lines 134 - 135, The check for infection transitions uses a hardcoded set infection_compartments = {"I", "E"} and the boolean is_infection_transition = source == "S" and target in infection_compartments; change this to consult a configurable source (e.g., a model-level attribute like model.infection_compartments or a transition metadata key such as transition.metadata.get("infection_targets")/transition.metadata.get("is_infection")) so the code uses the provided set/list of infected compartment names (falling back to a sensible default if absent). Update the logic around is_infection_transition to reference that configurable value (and allow staged names like "I1","I2" or full names like "Exposed") so network FOI is applied according to model/transition metadata rather than hardcoded names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patchsim/core/model.py`:
- Around line 38-42: The eval of rate_expr inside the try/except around rate_val
is currently only catching KeyError, NameError, and ValueError; extend the
exception handling in that block to also catch SyntaxError, TypeError,
ZeroDivisionError (and any other common eval runtime errors you deem necessary)
so malformed or invalid expressions get the same contextual error; when catching
these (e.g., in the except tuple alongside KeyError/NameError/ValueError) build
the same msg using rate_expr and transition_label and raise a ValueError(msg)
from the original exception so the original traceback is preserved; keep the
eval call using the restricted globals {"__builtins__": {}} and scope as before.
In `@src/patchsim/core/simulation.py`:
- Around line 71-76: The loop that builds network_matrix currently assumes every
network row's source/target exist in patch_idx, which will raise an unhelpful
KeyError; before using patch_idx in the loop over net_df.iterrows(), validate
that source and target (after .strip('"')) are present in patch_idx and if not
raise a clear ValueError naming the missing patch(es) and the offending row (use
the row's source/target values), otherwise proceed to check weight >= 0 and
assign network_matrix[i, j]; reference the symbols net_df.iterrows(), patch_idx,
network_matrix and the row["source"]/row["target"] lookups when locating where
to add this validation.
- Around line 119-124: The NetworkModel is not receiving canonical patch
ordering, so set net.patch_names to the ordered list used when building
patch_params; specifically, after creating net =
NetworkModel(base_model=base_model, num_patches=num_patches,
network_matrix=network_matrix) assign net.patch_parameters = patch_params and
then set net.patch_names = <the same ordered list of patch identifiers used to
construct patch_params> so compute_derivatives sees self.patch_names instead of
relying on dict ordering; reference NetworkModel, patch_parameters, and
patch_names in the change to ensure canonical ordering matches PatchFile input.
---
Duplicate comments:
In `@src/patchsim/core/model.py`:
- Around line 115-121: The patch parameter lookup currently uses
list(self.patch_parameters.keys()) which can mismatch the canonical patch order;
update the codeflow so simulation.py sets the canonical order by attaching the
patches list to the network (e.g., net.patch_names = patches), then change
model.py to use that canonical list (self.patch_names) when resolving
patch_params (fall back to base_model.parameters or
self.patch_parameters.get(patch_name) as before) instead of relying on dict key
order; reference the symbols patch_parameters, base_model.parameters,
PatchFile/paches list, and add/consume net.patch_names to ensure correct
mapping.
---
Nitpick comments:
In `@src/patchsim/core/model.py`:
- Around line 134-135: The check for infection transitions uses a hardcoded set
infection_compartments = {"I", "E"} and the boolean is_infection_transition =
source == "S" and target in infection_compartments; change this to consult a
configurable source (e.g., a model-level attribute like
model.infection_compartments or a transition metadata key such as
transition.metadata.get("infection_targets")/transition.metadata.get("is_infection"))
so the code uses the provided set/list of infected compartment names (falling
back to a sensible default if absent). Update the logic around
is_infection_transition to reference that configurable value (and allow staged
names like "I1","I2" or full names like "Exposed") so network FOI is applied
according to model/transition metadata rather than hardcoded names.
In `@src/patchsim/core/simulation.py`:
- Around line 163-164: Remove the dead commented-out call to
plot_patch_subplots: delete the commented line containing
"plot_patch_subplots(t_range, out_ode, patches, plots_dir, model_name)" so only
the active model.visualize(t_range, out_ode, patches, plots_dir, model_name)
call remains; this cleans up the code now that model.visualize handles plotting
and avoids confusion about duplicate plotting functions.
- Around line 25-29: The required_fields list currently contains ["PatchFile",
"SeedFile", "OutputDir"] but omits "Transitions" and "TMax", causing
inconsistent error styles; update the required_fields variable to include
"Transitions" and "TMax" so missing config keys raise the same "Missing required
field" ValueError; after adding them, ensure the later specific checks for
Transitions and TMax (the validation logic referencing those keys) either remain
as more detailed validation (not duplicate the missing-key error) or are
adjusted to assume presence and only validate content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0bc81ce-03e7-4c94-a412-73b32c0d0529
📒 Files selected for processing (4)
pyproject.tomlsrc/patchsim/cli.pysrc/patchsim/core/model.pysrc/patchsim/core/simulation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/patchsim/cli.py
- pyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/patchsim/core/model.py (2)
139-139: Redundanthasattr(self, "network")check.
self.networkis always set in__init__(line 63), sohasattr(self, "network")is alwaysTruefor anyNetworkModelinstance. Theself.network is not Nonecheck alone is sufficient.♻️ Suggested simplification
- has_network = self.num_patches > 1 and hasattr(self, "network") and self.network is not None + has_network = self.num_patches > 1 and self.network is not None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/core/model.py` at line 139, The condition computing has_network in NetworkModel is redundant; remove the hasattr(self, "network") check and set has_network = self.num_patches > 1 and self.network is not None instead. Locate the assignment to has_network (in method/constructor using self.num_patches and self.network) and simplify the boolean expression to rely only on self.num_patches > 1 and self.network is not None.
141-153: Consider consolidating FOI adjustment logic withmodel_runner.py.The FOI adjustment logic here (lines 141-153) is also implemented in
model_runner.py(lines 33-40 in context snippet). Both use the same formulabeta * S * lambdas[i]for infection transitions. Having two implementations increases the risk of divergence during future updates.Consider either:
- Having
model_runner.pydelegate toNetworkModel.compute_derivatives, or- Extracting the FOI adjustment logic into a shared helper method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patchsim/core/model.py` around lines 141 - 153, The FOI adjustment logic is duplicated between NetworkModel.compute_derivatives (the snippet in model.py that uses beta * S * lambdas[i] / FOI scaling) and the code in model_runner.py; extract that logic into a single shared helper (e.g., compute_foi_adjusted_rate or adjust_infection_rate) that accepts (patch_params, original_rate_expr, rate, patch_state, lambdas, i, is_infection_transition, has_network) and returns adjusted_rate, then replace the inline blocks in NetworkModel.compute_derivatives and the model_runner FOI block to call this helper; alternatively, have model_runner call NetworkModel.compute_derivatives directly so only one implementation remains — update both call sites to use the new helper or the single source-of-truth method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patchsim/core/model.py`:
- Around line 145-150: The substring check using "beta" on original_rate_expr is
too broad and can match names like "beta_max"; update the conditional in the
block that sets adjusted_rate (referencing original_rate_expr, adjusted_rate,
beta, lambdas, patch_state) to use a word-boundary regex match instead (e.g.,
use re.search with re.escape for "beta") consistent with the earlier pattern
used for source matching; ensure you import re if not already and preserve the
existing branches so only exact whole-word "beta" triggers the network FOI
correction.
- Around line 115-122: The code silently ignores patch-specific settings when
self.patch_parameters exists but self.patch_names is missing (patch_name ends up
None and self.patch_parameters.get(None, {}) is empty); update the logic in the
method that builds patch_params (where patch_name, patch_params and the hasattr
checks are used, e.g., in compute_derivatives or the surrounding block) to
detect this misconfiguration and either raise a clear ValueError or emit a
warning (choose project convention) stating that patch_parameters were provided
but patch_names is not set, and avoid silently falling back to base parameters;
ensure the message references self.patch_parameters and self.patch_names so
callers can quickly locate and fix the configuration.
---
Nitpick comments:
In `@src/patchsim/core/model.py`:
- Line 139: The condition computing has_network in NetworkModel is redundant;
remove the hasattr(self, "network") check and set has_network = self.num_patches
> 1 and self.network is not None instead. Locate the assignment to has_network
(in method/constructor using self.num_patches and self.network) and simplify the
boolean expression to rely only on self.num_patches > 1 and self.network is not
None.
- Around line 141-153: The FOI adjustment logic is duplicated between
NetworkModel.compute_derivatives (the snippet in model.py that uses beta * S *
lambdas[i] / FOI scaling) and the code in model_runner.py; extract that logic
into a single shared helper (e.g., compute_foi_adjusted_rate or
adjust_infection_rate) that accepts (patch_params, original_rate_expr, rate,
patch_state, lambdas, i, is_infection_transition, has_network) and returns
adjusted_rate, then replace the inline blocks in
NetworkModel.compute_derivatives and the model_runner FOI block to call this
helper; alternatively, have model_runner call NetworkModel.compute_derivatives
directly so only one implementation remains — update both call sites to use the
new helper or the single source-of-truth method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57757273-54ce-4c38-a460-8c681d1cc7c0
📒 Files selected for processing (3)
src/patchsim/core/model.pysrc/patchsim/core/simulation.pytests/test_config.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_config.py
- src/patchsim/core/simulation.py
snehasaisneha
requested changes
Apr 2, 2026
Member
snehasaisneha
left a comment
snehasaisneha
left a comment
There was a problem hiding this comment.
Hi @shreyamukherji - I've flagged a few specific issues with the CI flow, please resolve those.
I won't be able to go through the details of the business logic (as there are too many files and lots of changes), and so can only comment on the code's sanity and the CI/packaging aspects. Please make these revisions and I can review further.
.github/workflows/ci.yml
Outdated
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| python-version: ["3.11", "3.12", "3.13"] |
Member
There was a problem hiding this comment.
Python version should be picked up from the .python-version file in the root.
This is present in the other file as well.
.github/workflows/ci.yml
Outdated
| python-version: ${{ matrix.python-version }} | ||
|
|
||
| - name: Sync dependencies (including dev) | ||
| run: uv sync --extra dev |
Member
There was a problem hiding this comment.
uv sync --extra dev allows uv to re-sync the requirements in the root, which allows updates to creep in and can potentially alter functionality. Run uv sync locally before your commits. In CI, always use the following pattern:
# Step 1: Sync dependencies — fails if lockfile is out of date. This is a separate steps.
uv sync --extra dev --frozen
# Step 2: Run tools without re-syncing
uv run --no-sync --frozen ruff check .
Member
There was a problem hiding this comment.
This document expects users to be running patchsim directly in their root. The recommended pattern should be uv run patchsim --help to let the user utilise the right dependencies.
snehasaisneha
requested changes
Apr 6, 2026
.github/workflows/ci.yml
Outdated
| run: uv sync --frozen | ||
|
|
||
| - name: Build package | ||
| run: uv build |
Member
There was a problem hiding this comment.
This should also be uv build --no-sync --frozen
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.
This PR prepares patchsim for PyPI publication by addressing code hygiene, packaging, SDK usability, CLI structure, setting up CI/CD through github actions and documentation improvements.
It brings patchsim to a state ready for PyPI publication and external usage.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Validation
Tests
Chores