Conversation
📝 WalkthroughWalkthroughThis pull request introduces uniform random sampling capabilities to the biodivine_aeon library. New Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes introduce significant new functionality across 7 Rust binding modules with non-trivial sampling logic, projection mechanics, and retained variable handling. While patterns are similar across implementations, each module operates in a distinct context (colors, vertices, spaces, perturbations, and their colored variants), requiring careful verification of correctness for BDD-based uniform sampling, proper retained variable/function computation, and consistency across different type combinations. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
src/bindings/lib_param_bn/symbolic/set_color.rs (1)
275-293: Consider documenting the default seed behavior.The implementation correctly constructs the sampler with projection-aware uniform sampling. However,
seed.unwrap_or_default()defaults to0, which means callingsample_items()without a seed will always produce the same sequence. This is likely intentional for reproducibility, but worth documenting explicitly in the docstring.src/bindings/pbn_control/set_colored_perturbation.rs (1)
758-808: Consider extracting shared valuation-splitting logic.The
__next__implementation duplicates significant logic with_ColorPerturbationModelIterator::__next__(lines 702-743). Both iterate over network variables to separate color and perturbation valuations.This is acceptable for clarity, but if this pattern appears frequently, consider extracting a helper method.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Cargo.tomlbiodivine_aeon/__init__.pyiexample/script/uniform_random_sampling.pysrc/bindings/lib_param_bn/symbolic/set_color.rssrc/bindings/lib_param_bn/symbolic/set_colored_space.rssrc/bindings/lib_param_bn/symbolic/set_colored_vertex.rssrc/bindings/lib_param_bn/symbolic/set_spaces.rssrc/bindings/lib_param_bn/symbolic/set_vertex.rssrc/bindings/pbn_control/set_colored_perturbation.rssrc/bindings/pbn_control/set_perturbation.rstests/test_control_module.pytests/test_param_bn_module.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T14:52:22.035Z
Learnt from: daemontus
Repo: sybila/biodivine-aeon-py PR: 43
File: example/algorithms_evaluation/run.py:6-6
Timestamp: 2025-08-13T14:52:22.035Z
Learning: The file `example/algorithms_evaluation/run.py` is legacy code copied for experiments in the biodivine-aeon-py repository. Changes to this file are typically out of scope for PRs as it serves as reference/experimental code.
Applied to files:
example/script/uniform_random_sampling.py
🧬 Code graph analysis (9)
src/bindings/lib_param_bn/symbolic/set_spaces.rs (2)
src/bindings/lib_bdd/mod.rs (12)
module(14-14)module(15-15)module(16-16)module(17-17)module(18-18)module(19-19)module(20-20)module(21-21)module(22-22)module(23-23)module(24-24)module(25-25)src/bindings/lib_param_bn/symbolic/set_vertex.rs (3)
retained(389-395)compute_retained_variables(384-399)new(78-83)
src/bindings/lib_param_bn/symbolic/set_colored_space.rs (5)
src/bindings/lib_bdd/mod.rs (12)
module(14-14)module(15-15)module(16-16)module(17-17)module(18-18)module(19-19)module(20-20)module(21-21)module(22-22)module(23-23)module(24-24)module(25-25)src/bindings/lib_param_bn/symbolic/set_colored_vertex.rs (11)
compute_retained(343-366)ctx(447-449)new(83-88)sample_items(291-310)__iter__(137-139)__iter__(371-373)__iter__(408-410)__next__(375-398)__next__(412-438)next(401-403)next(441-443)src/bindings/lib_param_bn/symbolic/set_spaces.rs (11)
ctx(253-253)new(76-81)sample_items(232-248)compute_retained_variables(299-318)__iter__(126-128)__iter__(323-325)__iter__(341-343)__next__(327-331)__next__(345-357)next(334-336)next(360-362)src/bindings/lib_param_bn/symbolic/model_space.rs (1)
new_native(153-158)src/bindings/lib_param_bn/symbolic/model_color.rs (1)
new_native(329-341)
tests/test_control_module.py (8)
biodivine_aeon/__init__.pyi (9)
mk_unit_perturbations(1850-1850)sample_items(1166-1166)sample_items(1222-1222)sample_items(1274-1274)sample_items(1335-1339)sample_items(1383-1387)sample_items(1756-1759)sample_items(1825-1829)mk_unit_colored_perturbations(1852-1852)src/bindings/pbn_control/asynchronous_perturbation_graph.rs (2)
mk_unit_perturbations(354-356)mk_unit_colored_perturbations(365-368)src/bindings/pbn_control/set_colored_perturbation.rs (3)
sample_items(562-590)next(747-749)next(811-813)src/bindings/pbn_control/set_perturbation.rs (3)
sample_items(291-314)next(424-426)next(462-464)src/bindings/lib_param_bn/symbolic/set_colored_space.rs (3)
sample_items(286-306)next(427-429)next(476-478)src/bindings/lib_param_bn/symbolic/set_colored_vertex.rs (3)
sample_items(291-310)next(401-403)next(441-443)src/bindings/lib_param_bn/symbolic/set_vertex.rs (3)
sample_items(234-249)next(415-417)next(441-443)src/bindings/lib_param_bn/symbolic/set_color.rs (3)
sample_items(276-293)next(381-383)next(412-414)
tests/test_param_bn_module.py (5)
src/bindings/lib_param_bn/boolean_network.rs (1)
from_aeon(227-231)src/bindings/lib_param_bn/symbolic/asynchronous_graph.rs (1)
mk_subspace_vertices(447-453)src/bindings/lib_param_bn/symbolic/set_spaces.rs (2)
sample_items(232-248)ctx(253-253)src/bindings/lib_param_bn/symbolic/set_vertex.rs (3)
sample_items(234-249)retained(389-395)items(216-224)src/bindings/lib_param_bn/symbolic/symbolic_space_context.rs (2)
mk_unit_spaces(197-200)mk_unit_colored_spaces(172-191)
example/script/uniform_random_sampling.py (2)
src/lib.rs (1)
biodivine_aeon(84-95)biodivine_aeon/__init__.pyi (3)
BooleanNetwork(730-858)AsynchronousGraph(1391-1462)Reachability(1927-1943)
src/bindings/lib_param_bn/symbolic/set_color.rs (5)
src/bindings/lib_bdd/mod.rs (12)
module(14-14)module(15-15)module(16-16)module(17-17)module(18-18)module(19-19)module(20-20)module(21-21)module(22-22)module(23-23)module(24-24)module(25-25)src/bindings/lib_param_bn/symbolic/set_colored_space.rs (5)
ctx(189-189)ctx(315-315)ctx(400-400)ctx(449-449)new(84-89)src/bindings/lib_param_bn/symbolic/set_colored_vertex.rs (2)
ctx(447-449)new(83-88)src/bindings/lib_bdd/bdd_valuation.rs (4)
ctx(52-52)ctx(58-58)ctx(70-70)b(310-310)src/bindings/lib_param_bn/symbolic/model_color.rs (1)
new_native(329-341)
biodivine_aeon/__init__.pyi (7)
src/bindings/pbn_control/set_colored_perturbation.rs (1)
sample_items(562-590)src/bindings/pbn_control/set_perturbation.rs (1)
sample_items(291-314)src/bindings/lib_param_bn/symbolic/set_colored_space.rs (1)
sample_items(286-306)src/bindings/lib_param_bn/symbolic/set_colored_vertex.rs (1)
sample_items(291-310)src/bindings/lib_param_bn/symbolic/set_spaces.rs (1)
sample_items(232-248)src/bindings/lib_param_bn/symbolic/set_vertex.rs (2)
sample_items(234-249)retained(389-395)src/bindings/lib_param_bn/symbolic/set_color.rs (1)
sample_items(276-293)
src/bindings/lib_param_bn/symbolic/set_colored_vertex.rs (4)
src/bindings/lib_bdd/mod.rs (12)
module(14-14)module(15-15)module(16-16)module(17-17)module(18-18)module(19-19)module(20-20)module(21-21)module(22-22)module(23-23)module(24-24)module(25-25)src/bindings/lib_param_bn/symbolic/set_vertex.rs (7)
retained(389-395)new(78-83)compute_retained_variables(384-399)__next__(408-412)__next__(426-438)next(415-417)next(441-443)src/bindings/lib_param_bn/symbolic/set_colored_space.rs (10)
compute_retained(368-389)ctx(189-189)ctx(315-315)ctx(400-400)ctx(449-449)new(84-89)__next__(398-424)__next__(438-473)next(427-429)next(476-478)src/bindings/lib_param_bn/symbolic/set_color.rs (7)
ctx(352-352)new(87-92)read_retained_functions(318-360)__next__(369-378)__next__(392-409)next(381-383)next(412-414)
src/bindings/pbn_control/set_perturbation.rs (3)
src/bindings/lib_param_bn/symbolic/set_vertex.rs (4)
compute_retained_variables(384-399)retained(389-395)sample_items(234-249)new(78-83)src/bindings/pbn_control/set_colored_perturbation.rs (4)
ctx(646-651)ctx(652-656)sample_items(562-590)new(79-87)src/bindings/pbn_control/model_perturbation.rs (1)
new_native(237-250)
🪛 Ruff (0.14.10)
tests/test_param_bn_module.py
987-987: BooleanNetwork may be undefined, or defined from star imports
(F405)
995-995: AsynchronousGraph may be undefined, or defined from star imports
(F405)
1010-1010: BooleanExpression may be undefined, or defined from star imports
(F405)
1029-1029: SymbolicSpaceContext may be undefined, or defined from star imports
(F405)
1038-1038: AsynchronousGraph may be undefined, or defined from star imports
(F405)
example/script/uniform_random_sampling.py
1-1: from biodivine_aeon import * used; unable to detect undefined names
(F403)
7-7: BooleanNetwork may be undefined, or defined from star imports
(F405)
8-8: AsynchronousGraph may be undefined, or defined from star imports
(F405)
17-17: Reachability may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: tests-linux (3.9)
- GitHub Check: tests-linux (3.10)
- GitHub Check: tests-linux (3.12)
- GitHub Check: tests-windows
- GitHub Check: clippy
- GitHub Check: tests-linux (3.11)
- GitHub Check: tests-macos-and-coverage
- GitHub Check: mypy
- GitHub Check: notebooks-linux
- GitHub Check: tests-linux (3.9)
- GitHub Check: tests-linux (3.12)
- GitHub Check: tests-linux (3.11)
- GitHub Check: tests-linux (3.10)
- GitHub Check: tests-windows
- GitHub Check: tests-macos-and-coverage
- GitHub Check: notebooks-linux
- GitHub Check: clippy
- GitHub Check: mypy
- GitHub Check: pdoc documentation
🔇 Additional comments (30)
tests/test_control_module.py (1)
280-296: Well-structured tests for the new sampling functionality.The tests correctly verify:
- Seeded sampling produces consistent results
- Retained variable/function filtering excludes non-retained items from samples
- Both
PerturbationSetandColoredPerturbationSetsamplers work as expectedexample/script/uniform_random_sampling.py (1)
1-45: Good demonstration of the new sampling API.The example clearly illustrates the workflow for uniform random sampling and hypothesis testing. The use of
seed=1ensures reproducibility.One consideration: the relative path
'../case-study/butanol-production/butanol-pathway-f1-f2-f3.aeon'assumes a specific execution context. Consider adding a brief comment about where to run this script from, or usingPath(__file__).parentfor more robust path resolution.tests/test_param_bn_module.py (1)
986-1054: Comprehensive test coverage for symbolic samplers.The test effectively exercises:
VertexSet.sample_items()with constraint validationColorSet.sample_items()with function expression verificationColoredVertexSet.sample_items()combining bothSpaceSet.sample_items()with retained variable projectionColoredSpaceSet.items()with retained variables and functionsThe assertions properly verify that samples respect the underlying set constraints and that retained variable filtering excludes non-retained items.
src/bindings/lib_param_bn/symbolic/set_spaces.rs (3)
46-56: Clean sampler struct definition.The
_SpaceModelSamplerstruct appropriately holds the context, projection, and sampler state needed for uniform sampling.
224-248: Well-implementedsample_itemsmethod.The implementation correctly:
- Reuses
compute_retained_variablesto avoid code duplication withitems()- Creates a
RawProjectionfor the retained variables- Initializes the RNG with a deterministic seed (defaulting to 0)
339-363: Implementation correctly handles empty BDD sampling.The
random_valuation_samplemethod returnsOption<BddValuation>, properly returningNonewhen the BDD is empty. PyO3 automatically converts this to the correct Python iterator behavior (StopIteration). The pattern is consistent across the codebase and handles edge cases appropriately.src/bindings/pbn_control/set_perturbation.rs (4)
55-66: Appropriate sampler struct for perturbation sampling.The
_PerturbationModelSamplercorrectly includesparameter_mappingwhich is needed to handle the perturbation-specific logic of unsetting state variables when perturbations are inactive.
283-314: Correct implementation ofsample_itemsfor perturbations.The method properly:
- Reuses
compute_retained_variablesfor consistency withitems()- Applies the duplicate pruning BDD to ensure unique perturbation sampling
- Initializes the sampler with the pruned projection
435-459: Post-sampling cleanup logic is correctly implemented.The
__next__implementation properly mirrors the iterator's logic at lines 409-421:
- Samples a valuation from the projection
- Copies retained variables to a partial valuation
- Removes state variables when the perturbation parameter is
false(lines 447-452)This ensures sampled
PerturbationModelinstances are semantically correct.
369-400: Well-factored helper function.Extracting
compute_retained_variableseliminates duplication betweenitems()andsample_items()while maintaining the perturbation-specific logic for variable resolution and mapping.src/bindings/lib_param_bn/symbolic/set_vertex.rs (4)
48-58: Clean sampler struct definition.The
_VertexModelSamplerfollows the established pattern with context, projection, and sampler fields.
226-249: Well-documentedsample_itemsmethod.The docstring clearly explains:
- The optional projection behavior
- That sampling is uniform with respect to the projected set
- The seed parameter for deterministic sampling
382-399: Clean helper function implementation.The
compute_retained_variableshelper:
- Uses idiomatic Rust with iterator chaining
- Properly resolves
VariableIdTypeto BDD variables- Falls back to all state variables when
retainedisNone
420-444: Correct sampler iterator implementation.The
__next__method properly:
- Samples a full valuation from the projection BDD
- Extracts only the retained variables into a partial valuation
- Constructs a
VertexModelwith the sampled valuesCargo.toml (1)
31-37: Dependency updates support the new sampling functionality.The updates to
biodivine-lib-param-bn(0.7.0 → 0.7.1),biodivine-hctl-model-checker(0.3.4 → 0.3.5), andbiodivine-algo-bdd-scc(0.3.1 → 0.3.2) are necessary for themk_uniform_valuation_samplerAPI used across multiple binding modules in this PR.src/bindings/lib_param_bn/symbolic/set_color.rs (3)
49-61: LGTM! Well-structured sampler type.The
_ColorModelSamplerstruct correctly encapsulates all necessary state for uniform sampling: the context, projection, sampler, and retained variable lists. This mirrors the iterator pattern established by_ColorModelIterator.
392-409: LGTM! Correct sampling implementation.The
__next__implementation properly:
- Samples a full valuation from the BDD
- Extracts only retained variables into a partial valuation
- Constructs a
ColorModelwith the retained implicit/explicit listsThe pattern correctly handles the case where the BDD is empty (returns
None).
308-316: Good semantic equality implementation.Using XOR with limit 1 is an efficient way to check BDD equality - if the XOR produces any non-terminal node, the BDDs differ.
src/bindings/lib_param_bn/symbolic/set_colored_vertex.rs (3)
51-63: LGTM! Consistent sampler structure.The
_ColorVertexModelSamplercorrectly mirrors the iterator structure and follows the same pattern as_ColorModelSamplerin set_color.rs.
340-366: Good refactoring to share retained computation logic.The
compute_retainedhelper properly consolidates the logic previously duplicated betweenitemsand the newsample_items, correctly combining function variables fromColorSet::read_retained_functionswith state variables fromVertexSet::compute_retained_variables.
412-438: LGTM! Correct separation of color and state variables.The implementation correctly:
- Samples a valuation from the projected BDD
- Separates retained variables into color (parameter) and state buckets using
find_state_variable- Constructs separate
ColorModelandVertexModelinstancesThis ensures the returned models don't contain cross-contaminated variable assignments.
src/bindings/pbn_control/set_colored_perturbation.rs (3)
54-68: LGTM! Appropriate additional fields for perturbation handling.The
_ColorPerturbationModelSamplercorrectly includes theparameter_mappingfield needed to handle perturbation variables, consistent with_ColorPerturbationModelIterator.
574-578: Correct duplicate pruning before sampling.The implementation correctly applies
mk_duplicate_pruning_bddbefore creating the projection, ensuring uniform sampling respects the pruning semantics required for perturbation sets.
625-628: LGTM! Appropriate clippy allow for complex return type.The
#[allow(clippy::type_complexity)]is justified given the 4-tuple return type. This is an internal helper, so the complexity is acceptable.src/bindings/lib_param_bn/symbolic/set_colored_space.rs (3)
52-64: LGTM! Consistent sampler design.The
_ColorSpaceModelSamplerfollows the established pattern with appropriate context type (SymbolicSpaceContext).
366-389: Good helper extraction matching ColoredVertexSet pattern.The
compute_retainedhelper correctly delegates toColorSet::read_retained_functionsandSpaceSet::compute_retained_variables, maintaining consistency with the pattern inset_colored_vertex.rs.
438-473: LGTM! Correct Python context handling.The
__next__implementation correctly:
- Extracts the parent
SymbolicContextfromSymbolicSpaceContextforColorModelconstruction- Uses
all_extra_state_variables()for space variables (different from regular state variables)- Returns
PyResultto handle potential extraction errorsbiodivine_aeon/__init__.pyi (3)
1166-1166: LGTM! Correct type signature for ColorSet.sample_items.The stub correctly mirrors the
items()signature with the addition of the optionalseedparameter.
1335-1339: LGTM! Consistent type signatures across colored set types.The
sample_itemssignatures forColoredVertexSet,ColoredSpaceSet, andColoredPerturbationSetcorrectly:
- Match their corresponding
items()signatures- Add the optional
seedparameter- Return appropriate tuple iterators
Also applies to: 1383-1387, 1825-1829
1756-1759: LGTM! Correct PerturbationSet.sample_items signature.The type stub correctly reflects the simpler signature (no
retained_functionsparameter) matching the Rust implementation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
==========================================
+ Coverage 71.79% 72.07% +0.28%
==========================================
Files 106 106
Lines 11449 11768 +319
==========================================
+ Hits 8220 8482 +262
- Misses 3229 3286 +57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Release Notes
New Features
sample_items()method to symbolic set collections (ColorSet, VertexSet, SpaceSet, PerturbationSet, and colored variants) for uniform random sampling with optional seed parameter for reproducible results.Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.