✨ New kinetic scheme visualizer 🎨#395
Conversation
Extract kinetic scheme visualizer initial draft from https://github.com/anmolbhatia05/thesis/tree/cd6d1169db169da743097dd93b30d8d5580aebe5/kinetic-scheme-visualizer
Move project files by Anmol Bhatia into reference_only folder
+ extra notes from Ivo on why interactive nodes may be needed
but could be prettier
so as to not pollute the future context windows
Use ColorCode for the coloring of the edges + their labels
Add strict numeric validation to kinetic-scheme config fields. Fix ground-state bar sizing to account for per-node width overrides. Update tests for new validation and bar-span behavior. Sync dev/docs/notebook defaults and option descriptions with current implementation.
|
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 a kinetic‑scheme visualizer: k‑matrix transition extraction, a lightweight kinetic graph model, hierarchical/spring/manual layout algorithms, Matplotlib rendering APIs/configs, docs and example YAMLs/notebook, dev tooling tweaks, and comprehensive unit tests for parser, graph, layout, and plotting. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant API as show_kinetic_scheme / show_dataset_kinetic_scheme
participant Parser as extract_transitions / extract_dataset_transitions
participant Graph as KineticGraph.from_transitions
participant Layouter as compute_layout
participant Renderer as _render_kinetic_scheme
participant Matplot as Matplotlib
User->>API: call with megacomplex/dataset, model, parameters, config
API->>Parser: extract transitions (filter / omit)
Parser-->>API: list of Transition
API->>Graph: build KineticGraph from transitions
Graph-->>API: nodes & edges
API->>Layouter: compute node positions (hierarchical/spring/manual)
Layouter-->>API: NodePositions
API->>Renderer: render nodes, edges, labels, ground-state bars
Renderer->>Matplot: draw patches & texts on Axes
Matplot-->>API: Figure, Axes
API-->>User: return (Figure, Axes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #395 +/- ##
===========================================
+ Coverage 60.19% 70.34% +10.15%
===========================================
Files 30 36 +6
Lines 1570 2509 +939
Branches 215 393 +178
===========================================
+ Hits 945 1765 +820
- Misses 605 674 +69
- Partials 20 70 +50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR adds a new
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A["Model + Parameters"] --> B["Layer 1: _k_matrix_parser.py\nextract_transitions()"]
B -->|"list[Transition]"| C["Layer 2: _kinetic_graph.py\nKineticGraph.from_transitions()"]
C -->|"KineticGraph"| D["Layer 3: _layout.py\ncompute_layout()"]
D -->|"NodePositions"| E["Layer 4: plot_kinetic_scheme.py\n_render_kinetic_scheme()"]
E -->|"Figure, Axes"| F["matplotlib output"]
G["KineticSchemeConfig\n(Pydantic)"] --> E
G --> D
D --> D1["HIERARCHICAL\n(DAGs/simple cycles)"]
D --> D2["SPRING\n(complex cycles)"]
D --> D3["MANUAL\n(user positions)"]
Last reviewed commit: 4faa698 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assert isinstance(fig_or_none, Figure), "Axes must belong to a Figure" | ||
| fig = fig_or_none |
There was a problem hiding this comment.
assert in production code is unreliable
assert statements are stripped when Python runs with the -O (optimize) flag, meaning this check would silently vanish and fig would be assigned None, leading to an AttributeError later. Consider replacing with an explicit if / raise:
| assert isinstance(fig_or_none, Figure), "Axes must belong to a Figure" | |
| fig = fig_or_none | |
| if not isinstance(fig_or_none, Figure): | |
| raise TypeError("Axes must belong to a Figure") | |
| fig = fig_or_none |
| t_right = half_w / abs(dx) | ||
| t_left = half_w / abs(dx) | ||
| t_top = half_h / abs(dy) | ||
| t_bottom = half_h / abs(dy) |
There was a problem hiding this comment.
Redundant variables — t_left/t_right and t_top/t_bottom are identical
t_right and t_left are both computed as half_w / abs(dx) — they're always equal (same for t_top/t_bottom). The subsequent selection t_x = t_right if dx > 0 else t_left always picks the same value. This happens to be correct because the ray direction is already encoded in dx/dy (used at line 723), but the four-variable naming is misleading and suggests distinct values were intended. Consider simplifying:
| t_right = half_w / abs(dx) | |
| t_left = half_w / abs(dx) | |
| t_top = half_h / abs(dy) | |
| t_bottom = half_h / abs(dy) | |
| t_x = half_w / abs(dx) | |
| t_y = half_h / abs(dy) |
| decay_megacomplexes = [ | ||
| mc | ||
| for mc in megacomplexes | ||
| if hasattr(fill_item(model.megacomplex[mc], model, parameters), "get_k_matrix") | ||
| ] |
There was a problem hiding this comment.
Double fill_item call for each decay megacomplex
fill_item() is called here to check hasattr(..., "get_k_matrix"), and then extract_transitions (line 185) calls fill_item() again for each of these same megacomplexes. If fill_item is expensive (it resolves all parameter expressions), this doubles the work. Consider caching the filled megacomplexes or passing the pre-filtered list to avoid the redundant call.
| effective_figsize = config.figsize if config.figsize != DEFAULT_FIGSIZE else figsize | ||
| effective_title = config.title if config.title is not None else title |
There was a problem hiding this comment.
figsize comparison with != can shadow the function argument
When the user passes figsize=(10.0, 8.0) (which happens to equal DEFAULT_FIGSIZE) via the function argument while config.figsize also equals the default, effective_figsize is set from the function argument — which works. But if the user sets config.figsize to a custom value like (12.0, 6.0), that takes priority. The issue is the reverse: a user who explicitly passes config=KineticSchemeConfig() (all defaults) and a custom figsize to the function will get the expected behavior.
However, a user who explicitly sets config=KineticSchemeConfig(figsize=(10.0, 8.0)) intending to override the function-level figsize arg wouldn't be able to, since the comparison can't distinguish "explicitly set to default" from "never set." This is a minor UX edge case — consider documenting that config.figsize only takes priority when it differs from the default.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive kinetic scheme visualization module to pyglotaran-extras, enabling publication-quality rendering of decay schemes from pyglotaran models. The implementation follows a clean four-layer architecture: extraction from k-matrices, graph construction, layout computation, and matplotlib rendering.
Changes:
- New
pyglotaran_extras.inspect.kinetic_schememodule with public API (show_kinetic_scheme,show_dataset_kinetic_scheme,KineticSchemeConfig) - Comprehensive test suite with 459 tests across 4 test files
- Jupyter notebook documentation with real-world examples
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Adds pyglotaran>=0.7.4 to dev dependencies |
uv.lock |
Updates lock file with new dependency and removes some greenlet wheels |
pyglotaran_extras/inspect/kinetic_scheme/__init__.py |
Public API exports |
pyglotaran_extras/inspect/kinetic_scheme/_constants.py |
Named constants for defaults |
pyglotaran_extras/inspect/kinetic_scheme/_k_matrix_parser.py |
Layer 1: Extracts transitions from k-matrices |
pyglotaran_extras/inspect/kinetic_scheme/_kinetic_graph.py |
Layer 2: Lightweight directed graph structure |
pyglotaran_extras/inspect/kinetic_scheme/_layout.py |
Layer 3: Hierarchical, spring, and manual layout algorithms |
pyglotaran_extras/inspect/kinetic_scheme/plot_kinetic_scheme.py |
Layer 4: Matplotlib rendering and config |
tests/inspect/kinetic_scheme/*.py |
Comprehensive test suite (4 files, 459 tests) |
docs/notebooks/kinetic_scheme_visualization.ipynb |
Tutorial notebook with examples |
docs/notebooks/target_rcg_gcrcg_rcgcr_refine.yml |
Example model YAML |
docs/notebooks/target_rcg_gcrcg_rcgcr_refine-params.yml |
Example parameters YAML |
devdocs/kinetic_scheme/architecture.md |
Architecture documentation |
pyglotaran_extras/inspect/__init__.py |
Exports new functions |
pyglotaran_extras/__init__.py |
Top-level exports |
.pre-commit-config.yaml |
Updates actionlint version |
.gitignore |
Adds macOS and local config ignores |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dev = [ | ||
| "pyglotaran>=0.7.4", | ||
| { include-group = "docs" }, | ||
| { include-group = "test" }, | ||
| ] |
There was a problem hiding this comment.
The pyglotaran dependency is added only to the dev dependency group, but the main code in _k_matrix_parser.py imports from glotaran.model.item and glotaran.model (via TYPE_CHECKING). This means the kinetic scheme visualizer requires pyglotaran at runtime, not just for development/testing.
The dependency should be moved to the main dependencies list in pyproject.toml, or the module should be documented as requiring users to have pyglotaran installed separately.
| (rcgcr_r4, rcgcr_r4): rates.k44 | ||
| km_rcgcr_g: | ||
| matrix: | ||
| (rcgcr_g, rcgcr_g): ratesrcgcr.k55 |
There was a problem hiding this comment.
Inconsistent naming convention for rate parameter groups. The file uses ratesrcgcr (no delimiter) on line 174 while other rate groups use dots as delimiters (e.g., rates.k55, ratesgcrcg.k55).
For consistency, this should likely be ratesrcgcr.k55 or follow whatever the established convention is in the glotaran model specification.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@docs/notebooks/kinetic_scheme_visualization.ipynb`:
- Around line 40-46: The standalone plt.show() at the end of the import block
can produce an empty figure in some Jupyter backends; remove this plt.show()
line (or replace it with a notebook backend magic like %matplotlib inline at the
top) so that plot rendering relies on the notebook's automatic display of
figures produced by functions such as show_kinetic_scheme,
show_dataset_kinetic_scheme, or usages of KineticSchemeConfig rather than
forcing an empty display.
In `@docs/notebooks/target_rcg_gcrcg_rcgcr_refine.yml`:
- Around line 216-217: Remove the developer scratch notes and dangling
comment/question: delete the commented-out "exclude_from_normalize: [s7]" entry
and the following open question about "compartment" and "equal_area" so the
notebook YAML contains only finalized, user-facing configuration; ensure there
are no references to a non-existent compartment "s7" left in the file (search
for "exclude_from_normalize" and "s7") and keep only valid, documented
configuration options.
- Around line 257-271: The datasets block is missing entries for guide_rcg_r1,
guide_rcg_r2, guide_rcg_r3, and guide_rcg_r4 so they inherit the default weight;
update the datasets list in the same block that currently contains guide_rcg_g,
guide_gcrcg_r1...guide_rcgcr_g to include guide_rcg_r1, guide_rcg_r2,
guide_rcg_r3, and guide_rcg_r4 and give them the same value: 0.001 so all
guide_* datasets use the intended weight.
In `@pyglotaran_extras/inspect/kinetic_scheme/plot_kinetic_scheme.py`:
- Around line 392-398: Replace the brittle runtime assert in plot_kinetic_scheme
when resolving fig from ax with an explicit type check: after obtaining
fig_or_none = ax.get_figure(), verify with if not isinstance(fig_or_none,
Figure) and raise a descriptive TypeError (or ValueError) explaining "Axes must
belong to a Figure" so the public API enforces the invariant even under
optimized/interpreter flags; update the branch in plot_kinetic_scheme.py that
assigns fig to use this explicit check instead of assert.
- Around line 712-723: In _rect_edge_intersection, remove the redundant
intermediate variables t_right/t_left and t_top/t_bottom (they're both
half_w/abs(dx) and half_h/abs(dy)), and simplify by directly computing t_x =
half_w / abs(dx) and t_y = half_h / abs(dy), then t = min(t_x, t_y) and return
(cx + dx * t, cy + dy * t); update/remove the conditional branches that choose
between right/left and top/bottom since they are no-ops.
🧹 Nitpick comments (13)
docs/notebooks/target_rcg_gcrcg_rcgcr_refine-params.yml (1)
1-6: Inconsistent YAML key quoting style.Lines 2–6 (
inputs1group) use unquoted keys (vary: False), while the remaining groups use quoted keys ("vary": False,"expr": ...). Both are valid YAML, but mixing styles in the same file hurts readability.docs/notebooks/target_rcg_gcrcg_rcgcr_refine.yml (1)
1-7: Consider removing the commented-outresidual_functionon line 6.For a documentation notebook asset, the commented-out alternative (
non_negative_least_squares) adds noise. If it's meant as a hint for users, a descriptive comment would be clearer.pyglotaran_extras/__init__.py (1)
11-13:NodeStyleConfigis not re-exported at the top level.
pyglotaran_extras.inspect.kinetic_scheme.__init__exportsNodeStyleConfig, but it's missing from bothpyglotaran_extras.inspect.__init__and this top-level__init__. The notebook (line 366) imports it directly from the subpackage. For API consistency, consider re-exporting it here alongsideKineticSchemeConfig, or document that it must be imported from the subpackage.Proposed fix
from pyglotaran_extras.inspect.kinetic_scheme import KineticSchemeConfig +from pyglotaran_extras.inspect.kinetic_scheme import NodeStyleConfig from pyglotaran_extras.inspect.kinetic_scheme import show_dataset_kinetic_scheme from pyglotaran_extras.inspect.kinetic_scheme import show_kinetic_schemeAnd add
"NodeStyleConfig"to__all__.devdocs/kinetic_scheme/architecture.md (2)
9-11: Add language specifiers to fenced code blocks.Lines 9, 32, and 221 use fenced code blocks without a language identifier, which triggers markdownlint MD040. Use
textor an appropriate language identifier.Also, the file map (lines 222–237) places
devdocs/insidepyglotaran_extras/inspect/kinetic_scheme/, but this file actually lives at the repo root underdevdocs/kinetic_scheme/architecture.md. Consider correcting the map or separating it into two path sections.Also applies to: 32-49, 221-237
271-271: Hardcoded test count will become stale."251 tests across 4 test files" will drift as tests are added or removed. Consider using an approximate description (e.g., "~250 tests") or removing the specific count.
tests/inspect/kinetic_scheme/test_plot_kinetic_scheme.py (1)
114-259: Consider closing figures in tests to reduce memory pressure in CI.Each test in
TestShowKineticSchemecreates a new matplotlibFigurewithout closing it. In a large test suite this can accumulate significant memory. Consider adding apytestautouse fixture or explicitplt.close(fig)calls (or ayield-based fixture) to avoid leaked figures in CI.♻️ Example fixture
import matplotlib.pyplot as plt `@pytest.fixture`(autouse=True) def _close_figures(): yield plt.close("all")Place this at module level or in a
conftest.pyfor thekinetic_schemetest directory.pyglotaran_extras/inspect/kinetic_scheme/_k_matrix_parser.py (1)
178-189: Redundantfill_itemcalls for decay-type filtering.
extract_dataset_transitionscallsfill_itemon each megacomplex (line 182) to check forget_k_matrix, and thenextract_transitionscallsfill_itemagain (line 91 of the same file) for the same megacomplexes. For typical kinetic schemes (3–20 nodes) this is negligible, but it's worth noting.tests/inspect/kinetic_scheme/test_k_matrix_parser.py (1)
120-129: Placeholder test does not verify the intended behavior.
test_non_decay_megacomplex_raises_type_erroronly asserts that a valid decay megacomplex succeeds — it never exercises theTypeErrorpath for a non-decay megacomplex. Consider mocking a megacomplex withoutget_k_matrixor usingpytest.mark.skipwith a reason until a proper fixture is available.♻️ Sketch using unittest.mock
def test_non_decay_megacomplex_raises_type_error(self) -> None: - """Non-decay megacomplex without get_k_matrix raises TypeError.""" - # Create a model that has a coherent-artifact megacomplex - # For now, we test with a mock approach - # The sequential model only has decay megacomplexes, so we test - # that valid ones don't raise - transitions = extract_transitions( - "megacomplex_sequential_decay", SCHEME_SEQ.model, SCHEME_SEQ.parameters - ) - assert len(transitions) > 0 + """Non-decay megacomplex without get_k_matrix raises TypeError.""" + from unittest.mock import MagicMock, patch + + mock_mc = MagicMock(spec=[]) # no get_k_matrix + mock_mc.type = "coherent-artifact" + with patch( + "pyglotaran_extras.inspect.kinetic_scheme._k_matrix_parser.fill_item", + return_value=mock_mc, + ): + with pytest.raises(TypeError, match="does not support k-matrix"): + extract_transitions( + "megacomplex_sequential_decay", + SCHEME_SEQ.model, + SCHEME_SEQ.parameters, + )pyglotaran_extras/inspect/kinetic_scheme/_layout.py (3)
165-165: Simplify: replace lambda withmindirectly.
lambda c: min(c)is equivalent to justminhere, since each componentcis aset[str].♻️ Proposed fix
- return sorted(components, key=lambda c: min(c)) + return sorted(components, key=min)
537-555: Barycenter heuristic uses hash-based sort index instead of positional index.
_node_sort_indexreturnsfloat(hash(label) % 1000) / 1000.0, which is a hash-based proxy rather than the predecessor's actual position within its layer. Classic barycenter ordering uses the predecessor's index in its layer. For small kinetic schemes this is unlikely to matter, but it could produce suboptimal orderings for larger graphs with many nodes per layer.
466-481:_propagate_layersmay decrement in-degree multiple times for parallel edges.If
graph.successors(node)contains duplicates (from parallel edges between the same pair),in_degree[neighbor]will be decremented more than once per BFS visit. The<= 0guard on line 480 and theprocessedset make this safe in practice, but it's a subtle invariant to be aware of.pyglotaran_extras/inspect/kinetic_scheme/plot_kinetic_scheme.py (1)
185-257: Significant duplication betweenshow_kinetic_schemeandshow_dataset_kinetic_scheme.Both functions share identical logic for resolving
effective_figsize,effective_title,h_spacing, and the Layer 2–4 pipeline (graph → layout → render). Only the extraction call (Layer 1) differs. Consider extracting the shared logic into a private helper.♻️ Sketch
def _show_scheme( transitions: list[Transition], *, ax: Axes | None, config: KineticSchemeConfig, figsize: tuple[float, float], title: str | None, ) -> tuple[Figure, Axes]: effective_figsize = config.figsize if config.figsize != DEFAULT_FIGSIZE else figsize effective_title = config.title if config.title is not None else title h_spacing = config.horizontal_spacing if h_spacing <= 0: h_spacing = 3.0 * config.node_width graph = KineticGraph.from_transitions(transitions) algorithm = LayoutAlgorithm(config.layout_algorithm) positions = compute_layout(graph, algorithm, ...) return _render_kinetic_scheme(graph, positions, config, ax=ax, ...)Also applies to: 260-336
pyglotaran_extras/inspect/kinetic_scheme/_kinetic_graph.py (1)
148-158:add_edgedoes not validate that source/target nodes exist.The docstring states "Source and target nodes must already exist," but if this precondition is violated,
self._adjacency[edge.source]will raise aKeyError. Sincefrom_transitionsalways callsadd_nodebeforeadd_edge, this works in the current code path, but a defensive check would make the API safer for future callers.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pyglotaran_extras/inspect/kinetic_scheme/_k_matrix_parser.py`:
- Around line 178-182: The list comprehension that builds decay_megacomplexes
accesses model.megacomplex[mc] directly and can raise KeyError for undefined
labels; update the filter to first check that mc exists in model.megacomplex
(e.g., using "mc in model.megacomplex" or model.megacomplex.get(mc)) and if a
referenced mc is missing raise a descriptive ValueError matching the style from
extract_transitions (mentioning the missing megacomplex label and dataset
context) so the code fails with a clear message instead of a raw KeyError.
In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py`:
- Around line 441-481: The BFS layer propagation in _propagate_layers can
decrement in_degree multiple times for duplicate edges; update the function to
ignore duplicate (node, neighbor) edges by tracking seen edge pairs (e.g., a
local seen_edges set) and only decrement in_degree[neighbor] when (node,
neighbor) is not yet in seen_edges, adding the pair then; additionally guard
against negative in_degree by clamping to zero before the in_degree[neighbor] <=
0 check and ensure layer computation still uses layers[node] to update
layers[neighbor] as now (node,neighbor) duplicates are ignored.
- Around line 558-571: The _node_sort_index function uses Python's built-in
hash(label) which is randomized between runs; replace it with a deterministic
computation (e.g., compute a stable hash with hashlib like md5/sha256 of
label.encode() and convert to an integer then mod 1000, or derive the index from
the alphabetical rank within the current node set) so barycenter ordering is
repeatable; update the _node_sort_index implementation to use that deterministic
method and ensure it still returns a float in [0,1) as before.
🧹 Nitpick comments (2)
pyglotaran_extras/inspect/kinetic_scheme/_layout.py (1)
689-694: Hardcoded0.3threshold and uniform nudge for multiple successors.The x-proximity threshold
0.3(Line 693) is independent ofhorizontal_spacing— for large spacing values it may never trigger, and for very small spacing it could nudge nodes that aren't actually overlapping. Consider making it relative (e.g.,horizontal_spacing * 0.1).Also, if a parent has multiple compartment successors in the same column, they all receive the identical
nudge, stacking them at the same shifted x-position.♻️ Suggested improvement
- if abs(sx - px) < 0.3 and sy < py: - positions[successor_label] = (sx + nudge, sy) + if abs(sx - px) < horizontal_spacing * 0.1 and sy < py: + nudge_count = sum( + 1 + for s2 in graph.successors(parent_label) + if (s2_node := graph.nodes.get(s2)) is not None + and not s2_node.is_ground_state + and s2 in positions + and abs(positions[s2][0] - px) < horizontal_spacing * 0.1 + and positions[s2][1] < py + ) + positions[successor_label] = (sx + nudge * nudge_count, sy)docs/notebooks/target_rcg_gcrcg_rcgcr_refine.yml (1)
1-6: Minor: commented-outresidual_functionalternative on Line 6.If this is intentional as a documented alternative for users, that's fine. Otherwise, consider removing it to keep the shipped config clean.
take it or leave it ^^
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py`:
- Around line 89-122: compute_layout currently returns immediately for
LayoutAlgorithm.MANUAL and therefore skips positioning ground-state nodes and
avoiding overlap; update the MANUAL branch in compute_layout (where it calls
_manual_layout) to run _position_ground_state_nodes(graph, positions,
ground_state_offset) and _avoid_ground_state_arrow_overlap(graph, positions,
horizontal_spacing) on the positions returned by _manual_layout before
returning, so manual layouts include GS node positions consistently with
_spring_layout and _hierarchical_layout.
In `@tests/inspect/kinetic_scheme/test_k_matrix_parser.py`:
- Around line 122-131: The test named
test_non_decay_megacomplex_raises_type_error is misleading because it currently
calls extract_transitions on SCHEME_SEQ (a decay-only model) and asserts
success; change it to actually exercise the TypeError branch in _k_matrix_parser
(the non-decay megacomplex path) by constructing or mocking a model containing a
non-decay megacomplex (e.g., a coherent-artifact megacomplex) and then call
extract_transitions (or the parser entrypoint used there) inside a
pytest.raises(TypeError) assertion; alternatively if the intent was to validate
the success case, rename the test to reflect that (and keep the current
assertion) so the TypeError path remains covered by a new test that targets the
non-decay megacomplex.
🧹 Nitpick comments (3)
pyglotaran_extras/inspect/kinetic_scheme/_layout.py (2)
720-723: Hardcoded0.3threshold for "same column" detection.The value
0.3on Line 722 is a magic number. Consider extracting it to a named constant in_constants.pyfor clarity and easier tuning.
648-671: Ground-state positioning assumes single parent — reasonable but fragile.Line 666 uses
parents[0]without checking if there are multiple predecessors. For typical kinetic schemes each GS node has exactly one parent, but if future changes allow shared GS nodes, this would silently pick only the first parent's position.tests/inspect/kinetic_scheme/test_layout.py (1)
224-246: Manual layout tests should also verify ground-state node behavior.Given the issue flagged in
_layout.pywhere manual layout skips_position_ground_state_nodes, consider adding a test with a graph that has GS nodes (e.g.,_make_sequential_graph()) to verify whether GS nodes get positions in manual mode. This would serve as a regression test for whichever fix is chosen.
by CodeRabiit
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pyglotaran_extras/inspect/kinetic_scheme/_layout.py (2)
663-670: GS node with no positioned parent silently falls back to origin.If
parentsis empty orparents[0]isn't inpositions, the GS node lands at(0.0, -offset). This is a reasonable defensive fallback, but it could produce surprising visual results if the graph data is malformed. A debug-level log here could help troubleshooting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py` around lines 663 - 670, The ground-state placement loop silently falls back to origin when no positioned parent is found; update the loop in the block that iterates graph.ground_state_nodes() (variables: gs_node, graph.predecessors, positions, ground_state_offset) to emit a debug-level log including the gs_node.label and parent list when the fallback branch is taken so you can trace malformed graphs and surprising layouts; ensure you use the module/class logger (or create one if missing) and include contextual info (gs_node.label, parents, chosen fallback position) in the log message.
718-722: Magic number0.3for same-column detection.The threshold on line 721 is a hard-coded magic number. Consider extracting it as a module-level constant (e.g.,
_SAME_COLUMN_TOLERANCE = 0.3) for clarity and easier tuning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py` around lines 718 - 722, The code uses a hard-coded magic number 0.3 in the same-column check inside the layout logic (the if abs(sx - px) < 0.3 and sy < py: branch that assigns positions[successor_label] = (sx + nudge, sy)). Replace the literal with a module-level constant (suggested name _SAME_COLUMN_TOLERANCE = 0.3) defined near the top of pyglotaran_extras/inspect/kinetic_scheme/_layout.py, then use that constant in the conditional (abs(sx - px) < _SAME_COLUMN_TOLERANCE) so the tolerance is named, documented, and easy to tune.tests/inspect/kinetic_scheme/test_layout.py (2)
121-128:test_node_sort_index_is_deterministicre-implements the production logic.The test computes the expected value using the same
hashlib.md5logic as_node_sort_index, so it's essentially a tautological check. It does usefully verify the[0, 1)range constraint on line 128. Consider also testing that two different labels produce different indices, which would add more meaningful coverage.Suggested addition
def test_node_sort_index_is_deterministic(self) -> None: """Node sort index should use deterministic hashing.""" label = "species_2" digest = hashlib.md5(label.encode(), usedforsecurity=False).digest() expected = int.from_bytes(digest[:4], "big") / 4294967296.0 actual = _node_sort_index(label) assert actual == expected assert 0.0 <= actual < 1.0 + + def test_node_sort_index_distinct_labels(self) -> None: + """Different labels should produce different sort indices.""" + assert _node_sort_index("A") != _node_sort_index("B")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/inspect/kinetic_scheme/test_layout.py` around lines 121 - 128, The test currently re-implements _node_sort_index by computing the MD5 digest itself, making it tautological; change the test_node_sort_index_is_deterministic to (a) assert the returned value from _node_sort_index is within [0,1) as already done, and (b) add an assertion that two different labels (e.g., "species_2" and "species_3") produce different indices by calling _node_sort_index for each and asserting inequality, or alternatively compare against a hard-coded expected numeric value computed once externally; reference the test function test_node_sort_index_is_deterministic and the helper _node_sort_index when making this change.
101-110: Overlap test uses exact float equality — fragile for position checks.
pos_i != pos_jon line 110 would pass if two positions differ by a tiny epsilon. For a stronger overlap guarantee, consider checking minimum Euclidean distance:Slightly more robust alternative
- for i, pos_i in enumerate(compartment_positions): - for j, pos_j in enumerate(compartment_positions): - if i != j: - assert pos_i != pos_j + for i, pos_i in enumerate(compartment_positions): + for j in range(i + 1, len(compartment_positions)): + pos_j = compartment_positions[j] + dist = ((pos_i[0] - pos_j[0]) ** 2 + (pos_i[1] - pos_j[1]) ** 2) ** 0.5 + assert dist > 0.01, f"Nodes {i} and {j} overlap at {pos_i}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/inspect/kinetic_scheme/test_layout.py` around lines 101 - 110, The test test_no_overlapping_positions uses exact float equality to compare positions; change it to assert a minimum Euclidean distance between any two compartment positions instead: for each pair from compartment_nodes use positions (from compute_layout with LayoutAlgorithm.HIERARCHICAL) to compute the distance (sqrt((x1-x2)**2+(y1-y2)**2)) and assert that distance is greater than a small threshold (e.g. 1e-6 or a domain-appropriate epsilon) so near-equal floating positions are treated as overlapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py`:
- Around line 89-121: The earlier bug where MANUAL layout skipped ground-state
positioning is fixed by moving the calls to _position_ground_state_nodes and
_avoid_ground_state_arrow_overlap after the layout branch; ensure that all
branches (_manual_layout, _spring_layout, _hierarchical_layout) always assign a
positions dict before those calls, keep the horizontal_spacing sentinel
resolution (DEFAULT_NODE_WIDTH) intact, and preserve passing graph, positions,
ground_state_offset to _position_ground_state_nodes and graph, positions,
horizontal_spacing to _avoid_ground_state_arrow_overlap so ground-state nodes
and overlap nudging run for every algorithm.
- Around line 471-493: The duplicate-edge handling in _propagate_layers is
fixed: retain the seen_edges set and the in_degree clamp (max(0, ...)) in the
loop over successors to prevent processing the same edge twice and avoid
negative in-degree values; specifically ensure the code in _propagate_layers
continues to check edge_pair in seen_edges and back_edges, adds edge_pair to
seen_edges, updates layers[neighbor] only when new_layer > layers[neighbor],
decrements in_degree[neighbor] using in_degree[neighbor] = max(0,
in_degree[neighbor] - 1), and only appends neighbor to queue when
in_degree[neighbor] == 0.
- Around line 585-599: The implementation of _node_sort_index using
hashlib.md5(..., usedforsecurity=False) already provides deterministic,
repeatable sort indices across Python runs, so no code changes are needed—leave
the _node_sort_index function as-is (keep the md5 call with
usedforsecurity=False and the digest-to-float conversion).
In `@tests/inspect/kinetic_scheme/test_k_matrix_parser.py`:
- Around line 123-139: Test fix: ensure the test monkeypatches the exact
function used by extract_transitions so it returns an object without
get_k_matrix to trigger the TypeError; specifically, patch
"pyglotaran_extras.inspect.kinetic_scheme._k_matrix_parser.fill_item" to return
a SimpleNamespace (no get_k_matrix) and call extract_transitions(mc_label,
model, SCHEME_SEQ.parameters) asserting pytest.raises(TypeError, match="does not
support k-matrix extraction"); confirm the patch target string matches the
import path used by extract_transitions and keep the error match text unchanged.
---
Nitpick comments:
In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py`:
- Around line 663-670: The ground-state placement loop silently falls back to
origin when no positioned parent is found; update the loop in the block that
iterates graph.ground_state_nodes() (variables: gs_node, graph.predecessors,
positions, ground_state_offset) to emit a debug-level log including the
gs_node.label and parent list when the fallback branch is taken so you can trace
malformed graphs and surprising layouts; ensure you use the module/class logger
(or create one if missing) and include contextual info (gs_node.label, parents,
chosen fallback position) in the log message.
- Around line 718-722: The code uses a hard-coded magic number 0.3 in the
same-column check inside the layout logic (the if abs(sx - px) < 0.3 and sy <
py: branch that assigns positions[successor_label] = (sx + nudge, sy)). Replace
the literal with a module-level constant (suggested name _SAME_COLUMN_TOLERANCE
= 0.3) defined near the top of
pyglotaran_extras/inspect/kinetic_scheme/_layout.py, then use that constant in
the conditional (abs(sx - px) < _SAME_COLUMN_TOLERANCE) so the tolerance is
named, documented, and easy to tune.
In `@tests/inspect/kinetic_scheme/test_layout.py`:
- Around line 121-128: The test currently re-implements _node_sort_index by
computing the MD5 digest itself, making it tautological; change the
test_node_sort_index_is_deterministic to (a) assert the returned value from
_node_sort_index is within [0,1) as already done, and (b) add an assertion that
two different labels (e.g., "species_2" and "species_3") produce different
indices by calling _node_sort_index for each and asserting inequality, or
alternatively compare against a hard-coded expected numeric value computed once
externally; reference the test function test_node_sort_index_is_deterministic
and the helper _node_sort_index when making this change.
- Around line 101-110: The test test_no_overlapping_positions uses exact float
equality to compare positions; change it to assert a minimum Euclidean distance
between any two compartment positions instead: for each pair from
compartment_nodes use positions (from compute_layout with
LayoutAlgorithm.HIERARCHICAL) to compute the distance
(sqrt((x1-x2)**2+(y1-y2)**2)) and assert that distance is greater than a small
threshold (e.g. 1e-6 or a domain-appropriate epsilon) so near-equal floating
positions are treated as overlapping.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pyglotaran_extras/inspect/kinetic_scheme/_layout.py (2)
553-569: Barycenter uses hash-based sort indices rather than actual predecessor positions.The barycenter heuristic here computes averages of
_node_sort_index(p)(hash-based) rather than actual x-positions of predecessors from the previous layer. While this provides deterministic ordering, it doesn't minimize edge crossings in the traditional Sugiyama sense. For an initial implementation this is reasonable, but a follow-up could use actual computed positions from already-laid-out layers for better crossing reduction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py` around lines 553 - 569, Barycenter currently averages hash-based values from _node_sort_index rather than using actual x-positions of predecessor nodes, which reduces crossing-minimization quality; change the computation in the loop over nodes_in_layer so that for each predecessor p (from graph.predecessors, respecting compartment_labels, layers, and back_edges) you use the laid-out x-position of p (e.g., a node_positions or computed_positions map for nodes in earlier layers) to compute barycenters[label] = average(x_position(p)), falling back to _node_sort_index(p) only if the predecessor has no computed position; update references in the barycenter calculation and ensure node_positions is populated from prior layer layout before this loop so ordering uses actual predecessor geometry.
716-734: Overlap nudge is unidirectional — multiple nodes in the same column will stack.
_avoid_ground_state_arrow_overlapalways nudges tosx + nudge(positive x). If a parent node has a GS decay and multiple successors are positioned directly below it (same x-column), they'll all be nudged to the same x-coordinate, potentially overlapping each other.This is unlikely in typical kinetic schemes where a parent rarely has two non-GS successors at the same x, but worth noting for robustness. A cumulative nudge or alternating left/right could address it later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py` around lines 716 - 734, The overlap nudge in _avoid_ground_state_arrow_overlap currently always shifts successors to sx + nudge, causing multiple successors in the same column to collide; update the logic that iterates nodes_with_gs_decay and their successors (use graph.successors, positions, nodes_with_gs_decay, _SAME_COLUMN_TOLERANCE, nudge) to apply a per-successor offset instead of a fixed one — e.g., compute an index for each successor under the same parent and use alternating or cumulative offsets (sx ± nudge, sx + nudge * index) so successive nudges spread successors horizontally and avoid stacking.tests/inspect/kinetic_scheme/test_layout.py (1)
383-393: Consider adding a test for thehorizontal_spacingsentinel value (≤ 0 auto-compute).
compute_layouthas a sentinel check at Line 95 of_layout.pywherehorizontal_spacing <= 0triggers auto-computation fromDEFAULT_NODE_WIDTH. This path isn't explicitly tested. A test passinghorizontal_spacing=0(or the default) and verifying the layout still produces valid, non-overlapping positions would improve coverage of that branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/inspect/kinetic_scheme/test_layout.py` around lines 383 - 393, Add a new test alongside test_component_gap_respected that calls compute_layout(graph, LayoutAlgorithm.HIERARCHICAL, horizontal_spacing=0) (using the same _make_two_component_graph) to exercise the sentinel path where horizontal_spacing <= 0 triggers auto-computation from DEFAULT_NODE_WIDTH; assert the returned positions are valid by checking nodes within each component do not overlap horizontally (e.g., min x of right component > max x of left component) and that the inter-component gap is positive and reasonable (non-zero and finite), which verifies the auto-computed spacing path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pyglotaran_extras/inspect/kinetic_scheme/_layout.py`:
- Around line 553-569: Barycenter currently averages hash-based values from
_node_sort_index rather than using actual x-positions of predecessor nodes,
which reduces crossing-minimization quality; change the computation in the loop
over nodes_in_layer so that for each predecessor p (from graph.predecessors,
respecting compartment_labels, layers, and back_edges) you use the laid-out
x-position of p (e.g., a node_positions or computed_positions map for nodes in
earlier layers) to compute barycenters[label] = average(x_position(p)), falling
back to _node_sort_index(p) only if the predecessor has no computed position;
update references in the barycenter calculation and ensure node_positions is
populated from prior layer layout before this loop so ordering uses actual
predecessor geometry.
- Around line 716-734: The overlap nudge in _avoid_ground_state_arrow_overlap
currently always shifts successors to sx + nudge, causing multiple successors in
the same column to collide; update the logic that iterates nodes_with_gs_decay
and their successors (use graph.successors, positions, nodes_with_gs_decay,
_SAME_COLUMN_TOLERANCE, nudge) to apply a per-successor offset instead of a
fixed one — e.g., compute an index for each successor under the same parent and
use alternating or cumulative offsets (sx ± nudge, sx + nudge * index) so
successive nudges spread successors horizontally and avoid stacking.
In `@tests/inspect/kinetic_scheme/test_layout.py`:
- Around line 383-393: Add a new test alongside test_component_gap_respected
that calls compute_layout(graph, LayoutAlgorithm.HIERARCHICAL,
horizontal_spacing=0) (using the same _make_two_component_graph) to exercise the
sentinel path where horizontal_spacing <= 0 triggers auto-computation from
DEFAULT_NODE_WIDTH; assert the returned positions are valid by checking nodes
within each component do not overlap horizontally (e.g., min x of right
component > max x of left component) and that the inter-component gap is
positive and reasonable (non-zero and finite), which verifies the auto-computed
spacing path.
jsnel
force-pushed
the
new-kinetic-scheme-visualizer-2
branch
from
0e32062 to
f2a0bcf
Compare
and notebook documentation.
Credit for the initial work which was used for ideas and inspiration goes to Anmol Bhatia for his thesis work.
Change summary
kinetic_scheme_visualization.ipynb) illustrating the use caseChecklist
Screenshots
This allows for the visualization of simple sequential and parallel decays.
But potentially also much more complex kinetic schemes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores