✨ Enable Cut View on AIG#315
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 an immutable Cut view API (CutAig / Cut{Network}) with Python type stubs, C++ bindings, docs, and tests; exposes read/query operations, blocks mutating network operations on the view, and clears source-network visited flags before constructing the cut. ChangesCut View API (CutAig / Cut{Network})
Sequence Diagram(s)sequenceDiagram
participant Py as Python caller
participant Bind as C++ binding (Cut{Network})
participant Ntk as Base network (ntk)
participant CV as mockturtle::cut_view
Py->>+Bind: __init__(ntk, leaves, root)
Bind->>+Ntk: clear_visited_flags()
Bind->>+CV: construct_cut_view(ntk, leaves, root)
CV->>Ntk: traverse subgraph (collect nodes/pis/gates)
CV-->>-Bind: provide read-only view interfaces
Bind-->>-Py: return Cut{Network} (immutable view)
Note over Py,Bind: Mutating ops (create_*, clone_node) raise RuntimeError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Hi @marcelwa , I think there are settings issue on formatting since I run clang-format it will remove these changes in logic_networks.cpp, but if I run not -s lint it will indeed add these changes automatically.
There was a problem hiding this comment.
What changes do you mean exactly?
There was a problem hiding this comment.
Sorry I left this as a comment, it doesn't show inline. I mean the lines between 334-337. I didn't manually change this format, but I do think there's a conflict between clang-format and not -s lint on these line, try run one before another and change the order you'll see they make different decision.
There was a problem hiding this comment.
That's odd because nox -s lint simply calls clang-format. Can you make sure that you're using the same version of clang-format for both?
There was a problem hiding this comment.
Thanks, you are right. I now see how pre-commit works, it has its own packages requirement in .pre-commit-config.yaml. I indeed used an old version on my local. Thanks for pointing out.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/networks/test_cut_aig.py (1)
18-21: The node-based constructor overload is still untested.
create_pi()returnsAigSignal, so both of theseCutAig(...)calls are exercising the signal overload. Please add one case that passesaig.get_node(...)values so theSequence[int]/std::vector<Node>constructor path is actually covered.Also applies to: 49-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/networks/test_cut_aig.py` around lines 18 - 21, The tests currently pass AigSignal values (from create_pi()) into CutAig, so the Sequence[int]/std::vector<Node> constructor overload isn't exercised; add a parallel case that fetches Node objects via aig.get_node(...) and passes those to CutAig. Concretely, create leaves_nodes = [aig.get_node(x1.node), aig.get_node(x2.node)] and root_node = aig.get_node(f1.node) (or use the appropriate .node/index property available on the AigSignal), call CutAig(aig, leaves_nodes, root_node), and duplicate the same assertions; do the same for the other instance around lines 49-52 to cover both spots.
🤖 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/aigverse/networks/logic_networks.cpp`:
- Around line 541-556: The two cut_view constructors bound via nb::init<const
Ntk&, const std::vector<Node>&, const Signal&>() and nb::init<const Ntk&, const
std::vector<Signal>&, const Signal&>() must clear the network's mockturtle
visited flags before constructing mockturtle::cut_view; modify these binding
wrappers in logic_networks.cpp so they reset/clear the visited markers on the
provided ntk (or call an existing reset_visited/clear_visited method on Ntk)
immediately prior to invoking the cut_view constructors, or alternatively
provide and bind an explicit Ntk::reset_visited() method to Python and call it
from the constructors to ensure all visited flags are zeroed.
- Around line 533-618: The binding wrongly registers CutNtk as
nb::class_<CutNtk, Ntk>, exposing mutable methods from the full Ntk; change the
registration to a standalone type (nb::class_<CutNtk>(m, ...)) so it does not
inherit Ntk's API, or explicitly block/override mutable methods (e.g.,
create_and, create_pi, clone_node) to raise/return errors to match
mockturtle::immutable_view semantics; also update the two CutNtk constructors'
docstrings to document the visited-flag precondition (all nodes must have
visited==0 before construction and the view will reset visited flags to 0 after
construction).
---
Nitpick comments:
In `@test/networks/test_cut_aig.py`:
- Around line 18-21: The tests currently pass AigSignal values (from
create_pi()) into CutAig, so the Sequence[int]/std::vector<Node> constructor
overload isn't exercised; add a parallel case that fetches Node objects via
aig.get_node(...) and passes those to CutAig. Concretely, create leaves_nodes =
[aig.get_node(x1.node), aig.get_node(x2.node)] and root_node =
aig.get_node(f1.node) (or use the appropriate .node/index property available on
the AigSignal), call CutAig(aig, leaves_nodes, root_node), and duplicate the
same assertions; do the same for the other instance around lines 49-52 to cover
both spots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 746e4a21-10c4-4fe6-98e3-66ee316d2748
📒 Files selected for processing (4)
docs/aigs.mdpython/aigverse/networks.pyisrc/aigverse/networks/logic_networks.cpptest/networks/test_cut_aig.py
marcelwa
left a comment
marcelwa
left a comment
There was a problem hiding this comment.
Many thanks for the addition. It looks pretty nice already. I just have a few comments you can find below. Please also address the clang-tidy warning.
[Run clang-tidy: src/aigverse/networks/logic_networks.cpp#L592](https://github.com/marcelwa/aigverse/pull/315/files#annotation_47738436166)
implicit conversion changes signedness: 'int' to 'size_type' (aka 'unsigned long')
| cut_aig2 = CutAig(aig, [x1, x2, x3], f2) | ||
|
|
||
| print(f"\nLarger cut has {cut_aig2.num_pis} PIs (leaves)") | ||
| print(f"Larger cut has {cut_aig2.num_pos} PO (root)") |
There was a problem hiding this comment.
| print(f"Larger cut has {cut_aig2.num_pos} PO (root)") | |
| print(f"Larger cut has {cut_aig2.num_pos} POs (roots)") |
| print(f" Gate: {gate}") | ||
| ``` | ||
|
|
||
| You can also create larger cuts that include multiple gates: |
There was a problem hiding this comment.
The two examples are largely redundant. I would merge them into one that explores two different cuts in the same AIG.
| :::{note} | ||
| A cut is only valid if all dependencies of the root node are either included in the leaves or can be reached through nodes within the cut. The cut view assumes that all nodes' visited flags are set to 0 before creating the view. | ||
| ::: |
There was a problem hiding this comment.
We could simply reset the visited flags in the constructor.
| def fanouts(self, n: int) -> list[int]: | ||
| """Returns fanout nodes of node ``n``.""" | ||
|
|
||
| class CutAig(Aig): |
There was a problem hiding this comment.
Not sure if you saw: there is a new nox session called stubs that automatically generates the stubs from the bindings. It's not perfect yet, so it will try to overwrite some changes. I'll fix the configuration once I have time. In the meantime, please run it (if not already done) and don't commit the changes that are not caused by your edits.
There was a problem hiding this comment.
nox -s lint and nox -s stubs make different decisions since ruff involved, which one should have higher priority? ruff format failed in nox -s lint if you run stubs:
(venv) (base) ➜ aigverse git:(main) ✗ nox -s lint
nox > Running session lint
nox > Reusing existing virtual environment at .nox/lint.
nox > pre-commit run --all-files
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check vcs permalinks.....................................................Passed
check docstring is first.................................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check json...........................................(no files to check)Skipped
check toml...............................................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
fix end of files.........................................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed
Fix ligature characters with NFKD normalization..........................Passed
Fix smartquote characters................................................Passed
typos....................................................................Passed
cmake-format.............................................................Passed
clang-format.............................................................Passed
uv-lock..................................................................Passed
ruff format..............................................................Passed
ruff check...............................................................Passed
mypy.....................................................................Passed
nb-clean.............................................(no files to check)Skipped
blacken-docs.............................................................Passed
prettier.................................................................Passed
Validate GitHub Workflows................................................Passed
Validate ReadTheDocs Config..............................................Passed
Validate Renovate Config.................................................Passed
Validate pyproject.toml..................................................Passed
nox > Session lint was successful in 3 seconds.
(venv) (base) ➜ aigverse git:(main) ✗ nox -s stubs
nox > Running session stubs
nox > Reusing existing virtual environment at .nox/stubs.
nox > uv sync --no-dev --group build
Resolved 130 packages in 7ms
Built aigverse @ file:///xxx/aigverse
Prepared 1 package in 1m 02s
Uninstalled 1 package in 1ms
Installed 1 package in 1ms
~ aigverse==0.0.28.dev27+ge1cb0a5de.d20260323 (from file:///xxx/aigverse)
nox > python -m nanobind.stubgen --recursive --include-private --pattern-file /xxx/aigverse/python/aigverse/stubgen.pattern --output-dir /xxx/aigverse/python/aigverse --module aigverse.networks --module aigverse.algorithms --module aigverse.io --module aigverse.generators --module aigverse.utils
Using pattern file "/xxx/aigverse/stubgen.pattern" ..
- loaded 3 patterns.
Module "aigverse.networks" ..
- importing ..
- analyzing ..
- warning: rule 'algorithms.__prefix__' did not match any elements.
- warning: rule 'networks.__prefix__' did not match any elements.
- warning: rule 'rebalance_function: str' did not match any elements.
- applied 0 patterns.
- writing stub "/xxx/aigverse/python/aigverse/networks.pyi" ..
Module "aigverse.algorithms" ..
- importing ..
- analyzing ..
- warning: rule 'algorithms.__prefix__' did not match any elements.
- warning: rule 'networks.__prefix__' did not match any elements.
- warning: rule 'rebalance_function: str' did not match any elements.
- applied 0 patterns.
- writing stub "/xxx/aigverse/python/aigverse/algorithms.pyi" ..
Module "aigverse.io" ..
- importing ..
- analyzing ..
- warning: rule 'algorithms.__prefix__' did not match any elements.
- warning: rule 'networks.__prefix__' did not match any elements.
- warning: rule 'rebalance_function: str' did not match any elements.
- applied 0 patterns.
- writing stub "/xxx/aigverse/python/aigverse/io.pyi" ..
Module "aigverse.generators" ..
- importing ..
- analyzing ..
- warning: rule 'algorithms.__prefix__' did not match any elements.
- warning: rule 'networks.__prefix__' did not match any elements.
- warning: rule 'rebalance_function: str' did not match any elements.
- applied 0 patterns.
- writing stub "/xxx/aigverse/python/aigverse/generators.pyi" ..
Module "aigverse.utils" ..
- importing ..
- analyzing ..
- warning: rule 'algorithms.__prefix__' did not match any elements.
- warning: rule 'networks.__prefix__' did not match any elements.
- warning: rule 'rebalance_function: str' did not match any elements.
- applied 0 patterns.
- writing stub "/xxx/aigverse/python/aigverse/utils.pyi" ..
nox > pre-commit run ruff-format --files
...
...
ruff format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook
5 files reformatted, 1 file left unchanged
nox > pre-commit run ruff-check --files
...
...
ruff check...............................................................Failed
- hook id: ruff-check
- files were modified by this hook
Fixed 119 errors:
- python/aigverse/algorithms.pyi:
8 × D212 (multi-line-summary-first-line)
1 × D200 (unnecessary-multiline-docstring)
1 × I001 (unsorted-imports)
1 × E303 (too-many-blank-lines)
1 × F401 (unused-import)
- python/aigverse/generators.pyi:
7 × D212 (multi-line-summary-first-line)
- python/aigverse/io.pyi:
10 × D212 (multi-line-summary-first-line)
3 × D214 (overindented-section)
- python/aigverse/networks.pyi:
36 × D212 (multi-line-summary-first-line)
10 × PYI029 (str-or-repr-defined-in-stub)
9 × D214 (overindented-section)
8 × E303 (too-many-blank-lines)
4 × D200 (unnecessary-multiline-docstring)
1 × I001 (unsorted-imports)
1 × W391 (too-many-newlines-at-end-of-file)
- python/aigverse/utils.pyi:
16 × D212 (multi-line-summary-first-line)
1 × PYI029 (str-or-repr-defined-in-stub)
1 × E303 (too-many-blank-lines)
Found 119 errors (119 fixed, 0 remaining).
nox > pre-commit run ruff-check --files
...
...
ruff check...............................................................Passed
nox > Session stubs was successful in a minute.
(venv) (base) ➜ aigverse git:(main) ✗ nox -s lint
nox > Running session lint
nox > Reusing existing virtual environment at .nox/lint.
nox > pre-commit run --all-files
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check vcs permalinks.....................................................Passed
check docstring is first.................................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check json...........................................(no files to check)Skipped
check toml...............................................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
fix end of files.........................................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed
Fix ligature characters with NFKD normalization..........................Passed
Fix smartquote characters................................................Passed
typos....................................................................Passed
cmake-format.............................................................Passed
clang-format.............................................................Passed
uv-lock..................................................................Passed
**ruff format..............................................................Failed**
- hook id: ruff-format
- files were modified by this hook
1 file reformatted, 37 files left unchanged
ruff check...............................................................Passed
mypy.....................................................................Passed
nb-clean.............................................(no files to check)Skipped
blacken-docs.............................................................Passed
prettier.................................................................Passed
Validate GitHub Workflows................................................Passed
Validate ReadTheDocs Config..............................................Passed
Validate Renovate Config.................................................Passed
Validate pyproject.toml..................................................Passed
nox > Command pre-commit run --all-files failed with exit code 1
nox > Session lint failed.There was a problem hiding this comment.
I will have to come back to you regarding the stubs. As said, I'm not 100% happy with how stubgen currently generates the stubs. Once I fix that, the behavior should no longer be conflicting.
There was a problem hiding this comment.
What changes do you mean exactly?
| "pos", | ||
| [](const CutNtk& ntk) | ||
| { | ||
| std::vector<Signal> pos; |
There was a problem hiding this comment.
| std::vector<Signal> pos; | |
| std::vector<Signal> pos{}; |
|
|
||
|
|
||
| def test_cut_aig_with_signals() -> None: | ||
| """Test CutAig creation using signals instead of nodes.""" |
There was a problem hiding this comment.
What do you mean by "signals instead of nodes"? The previous test case also passed signals. You must call .get_node() if you want to test node construction.
There was a problem hiding this comment.
Add another test later with another constructor for "nodes instead of signals", now 2 are all tested. Done.
| nodes = list(cut_aig.nodes()) | ||
| assert len(nodes) == cut_aig.size | ||
| assert 0 in nodes # constant node | ||
|
|
||
| # Iterate over PIs | ||
| pis = list(cut_aig.pis()) | ||
| assert len(pis) == cut_aig.num_pis | ||
|
|
||
| # Iterate over gates | ||
| gates = list(cut_aig.gates()) | ||
| assert len(gates) == cut_aig.num_gates | ||
|
|
||
| # Iterate over POs | ||
| pos = list(cut_aig.pos()) | ||
| assert len(pos) == 1 |
There was a problem hiding this comment.
No need to explicitly convert to lists. The functions return lists already.
There was a problem hiding this comment.
The current tests don't cover the methods deleted by mockturtle::immutable_view. Those should be tested for as well.
There was a problem hiding this comment.
Tested all with customised run time error.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/networks/test_cut_aig.py (2)
32-50: Clarify the test docstring.The docstring says "using signals instead of nodes," but
test_cut_aigalso passes signals. The actual distinction is thattest_cut_aig_with_nodes(line 142) tests the node-based constructor overload usingget_node(). Consider updating the docstring to describe what this test uniquely covers (e.g., a more complex cut with multiple gates).📝 Suggested docstring
def test_cut_aig_with_signals() -> None: - """Test CutAig creation using signals instead of nodes.""" + """Test CutAig creation with a larger cut containing multiple gates."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/networks/test_cut_aig.py` around lines 32 - 50, Update the docstring for test_cut_aig_with_signals to clearly state that this test exercises the CutAig constructor that accepts signal objects (not nodes) and verifies a nontrivial cut composed of multiple gates (creates two ANDs and an XOR, 4 PIs and 5 gates); reference the signal-based constructor usage of CutAig and contrast it with test_cut_aig_with_nodes which uses get_node() to exercise the node-based overload.
159-198: Consider adding tests for nary methods.The immutability test covers most blocked methods but is missing
create_nary_and,create_nary_or, andcreate_nary_xor, which are also blocked in the C++ bindings.🧪 Additional assertions for completeness
with pytest.raises(RuntimeError, match="clone_node is not available on immutable view"): cut_aig.clone_node(aig, aig.get_node(x1), [x1]) + with pytest.raises(RuntimeError, match="create_nary_and is not available on immutable view"): + cut_aig.create_nary_and([x1, x2]) + with pytest.raises(RuntimeError, match="create_nary_or is not available on immutable view"): + cut_aig.create_nary_or([x1, x2]) + with pytest.raises(RuntimeError, match="create_nary_xor is not available on immutable view"): + cut_aig.create_nary_xor([x1, x2])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/networks/test_cut_aig.py` around lines 159 - 198, The test test_cut_aig_immutability is missing assertions for the n-ary operations; add pytest.raises checks to assert that calling CutAig.create_nary_and(...), CutAig.create_nary_or(...), and CutAig.create_nary_xor(...) raise RuntimeError with the same "is not available on immutable view" message (match string like the other assertions), using appropriate argument lists (e.g., [x1, x2] or a list of inputs) and keeping them alongside the existing create_* tests in the CutAig immutability block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/networks/test_cut_aig.py`:
- Around line 32-50: Update the docstring for test_cut_aig_with_signals to
clearly state that this test exercises the CutAig constructor that accepts
signal objects (not nodes) and verifies a nontrivial cut composed of multiple
gates (creates two ANDs and an XOR, 4 PIs and 5 gates); reference the
signal-based constructor usage of CutAig and contrast it with
test_cut_aig_with_nodes which uses get_node() to exercise the node-based
overload.
- Around line 159-198: The test test_cut_aig_immutability is missing assertions
for the n-ary operations; add pytest.raises checks to assert that calling
CutAig.create_nary_and(...), CutAig.create_nary_or(...), and
CutAig.create_nary_xor(...) raise RuntimeError with the same "is not available
on immutable view" message (match string like the other assertions), using
appropriate argument lists (e.g., [x1, x2] or a list of inputs) and keeping them
alongside the existing create_* tests in the CutAig immutability block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b917446-2be2-4182-9004-913f32dfdd56
📒 Files selected for processing (4)
docs/aigs.mdpython/aigverse/networks.pyisrc/aigverse/networks/logic_networks.cpptest/networks/test_cut_aig.py
✅ Files skipped from review due to trivial changes (1)
- docs/aigs.md
🚧 Files skipped from review as they are similar to previous changes (1)
- python/aigverse/networks.pyi
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/networks/test_cut_aig.py (1)
159-204: Refactor immutability checks with parametrization to reduce repetition.The current test is correct but repetitive; a parametrized table makes it easier to maintain when APIs change.
♻️ Proposed refactor
def test_cut_aig_immutability() -> None: """Test that network-modifying methods raise RuntimeError on CutAig.""" aig = Aig() x1 = aig.create_pi() x2 = aig.create_pi() f1 = aig.create_and(x1, x2) aig.create_po(f1) cut_aig = CutAig(aig, [x1, x2], f1) - with pytest.raises(RuntimeError, match="create_pi is not available on immutable view"): - cut_aig.create_pi() - with pytest.raises(RuntimeError, match="create_po is not available on immutable view"): - cut_aig.create_po(f1) - with pytest.raises(RuntimeError, match="create_and is not available on immutable view"): - cut_aig.create_and(x1, x2) - with pytest.raises(RuntimeError, match="create_or is not available on immutable view"): - cut_aig.create_or(x1, x2) - with pytest.raises(RuntimeError, match="create_xor is not available on immutable view"): - cut_aig.create_xor(x1, x2) - with pytest.raises(RuntimeError, match="create_not is not available on immutable view"): - cut_aig.create_not(x1) - with pytest.raises(RuntimeError, match="create_nand is not available on immutable view"): - cut_aig.create_nand(x1, x2) - with pytest.raises(RuntimeError, match="create_nor is not available on immutable view"): - cut_aig.create_nor(x1, x2) - with pytest.raises(RuntimeError, match="create_xnor is not available on immutable view"): - cut_aig.create_xnor(x1, x2) - with pytest.raises(RuntimeError, match="create_buf is not available on immutable view"): - cut_aig.create_buf(x1) - with pytest.raises(RuntimeError, match="create_maj is not available on immutable view"): - cut_aig.create_maj(x1, x2, f1) - with pytest.raises(RuntimeError, match="create_ite is not available on immutable view"): - cut_aig.create_ite(x1, x2, f1) - with pytest.raises(RuntimeError, match="create_xor3 is not available on immutable view"): - cut_aig.create_xor3(x1, x2, f1) - with pytest.raises(RuntimeError, match="create_lt is not available on immutable view"): - cut_aig.create_lt(x1, x2) - with pytest.raises(RuntimeError, match="create_le is not available on immutable view"): - cut_aig.create_le(x1, x2) - with pytest.raises(RuntimeError, match="create_nary_and is not available on immutable view"): - cut_aig.create_nary_and([x1, x2]) - with pytest.raises(RuntimeError, match="create_nary_or is not available on immutable view"): - cut_aig.create_nary_or([x1, x2]) - with pytest.raises(RuntimeError, match="create_nary_xor is not available on immutable view"): - cut_aig.create_nary_xor([x1, x2]) - with pytest.raises(RuntimeError, match="clone_node is not available on immutable view"): - cut_aig.clone_node(aig, aig.get_node(x1), [x1]) + cases = [ + ("create_pi", ()), + ("create_po", (f1,)), + ("create_and", (x1, x2)), + ("create_or", (x1, x2)), + ("create_xor", (x1, x2)), + ("create_not", (x1,)), + ("create_nand", (x1, x2)), + ("create_nor", (x1, x2)), + ("create_xnor", (x1, x2)), + ("create_buf", (x1,)), + ("create_maj", (x1, x2, f1)), + ("create_ite", (x1, x2, f1)), + ("create_xor3", (x1, x2, f1)), + ("create_lt", (x1, x2)), + ("create_le", (x1, x2)), + ("create_nary_and", ([x1, x2],)), + ("create_nary_or", ([x1, x2],)), + ("create_nary_xor", ([x1, x2],)), + ("clone_node", (aig, aig.get_node(x1), [x1])), + ] + + for method_name, args in cases: + with pytest.raises( + RuntimeError, + match=rf"{method_name} is not available on immutable view", + ): + getattr(cut_aig, method_name)(*args)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/networks/test_cut_aig.py` around lines 159 - 204, Refactor test_cut_aig_immutability to remove repetitive with pytest.raises blocks by parametrizing the immutable methods: create_pi, create_po, create_and, create_or, create_xor, create_not, create_nand, create_nor, create_xnor, create_buf, create_maj, create_ite, create_xor3, create_lt, create_le, create_nary_and, create_nary_or, create_nary_xor, clone_node; implement a pytest.mark.parametrize that supplies the method name (string) and its argument tuple, then inside the test use getattr(cut_aig, method_name)(*args) within pytest.raises(RuntimeError, match=f"{method_name} is not available on immutable view") to assert the error for each case while keeping the existing setup using Aig(), create_pi/create_and/create_po, and CutAig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/networks/test_cut_aig.py`:
- Around line 159-204: Refactor test_cut_aig_immutability to remove repetitive
with pytest.raises blocks by parametrizing the immutable methods: create_pi,
create_po, create_and, create_or, create_xor, create_not, create_nand,
create_nor, create_xnor, create_buf, create_maj, create_ite, create_xor3,
create_lt, create_le, create_nary_and, create_nary_or, create_nary_xor,
clone_node; implement a pytest.mark.parametrize that supplies the method name
(string) and its argument tuple, then inside the test use getattr(cut_aig,
method_name)(*args) within pytest.raises(RuntimeError, match=f"{method_name} is
not available on immutable view") to assert the error for each case while
keeping the existing setup using Aig(), create_pi/create_and/create_po, and
CutAig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35de727a-453c-4030-b08d-58d007b266c6
📒 Files selected for processing (1)
test/networks/test_cut_aig.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/networks/test_cut_aig.py (2)
159-195: Consider parametrizing immutability cases for clearer failure granularity.Current loop works, but
@pytest.mark.parametrizewould report each blocked method as an individual test case, making failures easier to triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/networks/test_cut_aig.py` around lines 159 - 195, The test test_cut_aig_immutability currently iterates a list of (method_name, args) and asserts RuntimeError for each, which hides per-case granularity; refactor it to a parametrized test using `@pytest.mark.parametrize` to supply the (method_name, args) pairs to a single test function so each blocked method is reported as an individual test case. Update test_cut_aig_immutability to be decorated with pytest.mark.parametrize (e.g., params = [("create_pi", ()), ...]) and accept method_name and args as parameters, then inside the test call getattr(CutAig(aig, [x1, x2], f1), method_name)(*args) within pytest.raises(RuntimeError, match=rf"{method_name} is not available on immutable view"); optionally add ids for clarity.
23-26: Remove redundanthasattrassertions.The direct value assertions on Line [27]–Line [29] already fail if these properties don’t exist, so these checks add noise without extra signal.
♻️ Suggested cleanup
- assert hasattr(cut_aig, "size") - assert hasattr(cut_aig, "num_gates") - assert hasattr(cut_aig, "num_pis") - assert hasattr(cut_aig, "num_pos") assert cut_aig.num_pis == 2 assert cut_aig.num_pos == 1 assert cut_aig.num_gates == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/networks/test_cut_aig.py` around lines 23 - 26, Remove the redundant hasattr checks for cut_aig ("size", "num_gates", "num_pis", "num_pos") in the test; the subsequent direct assertions already will raise if those attributes are missing, so delete the four lines asserting hasattr(cut_aig, "size"), hasattr(cut_aig, "num_gates"), hasattr(cut_aig, "num_pis"), and hasattr(cut_aig, "num_pos") and keep the direct value assertions only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/networks/test_cut_aig.py`:
- Around line 159-195: The test test_cut_aig_immutability currently iterates a
list of (method_name, args) and asserts RuntimeError for each, which hides
per-case granularity; refactor it to a parametrized test using
`@pytest.mark.parametrize` to supply the (method_name, args) pairs to a single
test function so each blocked method is reported as an individual test case.
Update test_cut_aig_immutability to be decorated with pytest.mark.parametrize
(e.g., params = [("create_pi", ()), ...]) and accept method_name and args as
parameters, then inside the test call getattr(CutAig(aig, [x1, x2], f1),
method_name)(*args) within pytest.raises(RuntimeError, match=rf"{method_name} is
not available on immutable view"); optionally add ids for clarity.
- Around line 23-26: Remove the redundant hasattr checks for cut_aig ("size",
"num_gates", "num_pis", "num_pos") in the test; the subsequent direct assertions
already will raise if those attributes are missing, so delete the four lines
asserting hasattr(cut_aig, "size"), hasattr(cut_aig, "num_gates"),
hasattr(cut_aig, "num_pis"), and hasattr(cut_aig, "num_pos") and keep the direct
value assertions only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4077d9c4-1fee-43bf-938c-5b01b8b513a9
📒 Files selected for processing (1)
test/networks/test_cut_aig.py
|
@wjrforcyber in #336 I fixed the stubs generation with |
Signed-off-by: JingrenWang <wjrforcyber@163.com>
Signed-off-by: JingrenWang <wjrforcyber@163.com>
Signed-off-by: JingrenWang <wjrforcyber@163.com>
wjrforcyber
force-pushed
the
cut_view
branch
2 times, most recently
from
832d432 to
ee38808
Compare
Signed-off-by: JingrenWang <wjrforcyber@163.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/aigverse/networks.pyi`:
- Around line 580-635: The blocked mutating methods in the AIG stubs (create_pi,
create_po, create_buf, create_not, create_and, create_nand, create_or,
create_nor, create_xor, create_xnor, create_lt, create_le, create_maj,
create_ite, create_xor3, create_nary_and, create_nary_or, create_nary_xor, and
clone_node) always raise and should be annotated with typing.NoReturn rather
than their original return types; add NoReturn to the typing imports (matching
the pattern used by SequentialAig.to_index_list) and change each method
signature's return type to NoReturn so type checkers know they never return.
🪄 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: db14eb32-b4e0-4d55-99d9-4c15c6bf90bc
📒 Files selected for processing (4)
docs/aigs.mdpython/aigverse/networks.pyisrc/aigverse/networks/logic_networks.cpptest/networks/test_cut_aig.py
Signed-off-by: JingrenWang <wjrforcyber@163.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcelwa
left a comment
marcelwa
left a comment
There was a problem hiding this comment.
This looks much better now, thanks a lot for your efforts 🙏
I'd like to discuss the CutAig class in general before we continue work on this PR. Please find my notes on that below. It's important to get the design right the first time to not introduce too many breaking changes down the line.
|
|
||
| using CutNtk = mockturtle::cut_view<Ntk>; | ||
|
|
||
| auto clear_visited = [](const Ntk& ntk) { ntk.foreach_node([&ntk](const auto& n) { ntk.set_visited(n, 0); }); }; |
There was a problem hiding this comment.
| auto clear_visited = [](const Ntk& ntk) { ntk.foreach_node([&ntk](const auto& n) { ntk.set_visited(n, 0); }); }; | |
| const auto clear_visited = [](const Ntk& ntk) { ntk.foreach_node([&ntk](const auto& n) { ntk.set_visited(n, 0); }); }; |
| the view will have a 0 visited flag after construction.)pb") | ||
| .def( | ||
| "__init__", | ||
| [clear_visited](CutNtk* self, const Ntk& ntk, const std::vector<Node>& leaves, const Signal& root) |
There was a problem hiding this comment.
| [clear_visited](CutNtk* self, const Ntk& ntk, const std::vector<Node>& leaves, const Signal& root) | |
| [&clear_visited](CutNtk* self, const Ntk& ntk, const std::vector<Node>& leaves, const Signal& root) |
| root: The root signal (output) of the cut.)pb") | ||
| .def( | ||
| "__init__", | ||
| [clear_visited](CutNtk* self, const Ntk& ntk, const std::vector<Signal>& leaves, const Signal& root) |
There was a problem hiding this comment.
| [clear_visited](CutNtk* self, const Ntk& ntk, const std::vector<Signal>& leaves, const Signal& root) | |
| [&clear_visited](CutNtk* self, const Ntk& ntk, const std::vector<Signal>& leaves, const Signal& root) |
| nb::arg("index"), R"pb(Returns the node for an index.)pb") | ||
| .def( | ||
| "__repr__", | ||
| [network_name](const CutNtk& ntk) |
There was a problem hiding this comment.
| [network_name](const CutNtk& ntk) | |
| [&network_name](const CutNtk& ntk) |
| nb::class_<CutNtk, Ntk>(m, fmt::format("Cut{}", network_name).c_str(), | ||
| R"pb(Implements an isolated view on a single cut in a network. | ||
|
|
||
| This view creates a network from a single cut with a single output `root` | ||
| and a set of `leaves`. This is an immutable view; network-modifying methods | ||
| are not available. | ||
|
|
||
| Note: | ||
| This view clears all nodes' visited flags before construction to ensure | ||
| the cut is constructed correctly. The view guarantees that all nodes in | ||
| the view will have a 0 visited flag after construction.)pb") |
There was a problem hiding this comment.
Let us briefly pause and think about cuts in aigverse for a second. Currently, this PR exposes mockturtle::cut_view directly and makes it a new network type in aigverse. That means you have to explicitly disable all the regular network functions because of the inheritance from mockturtle::immutable_view, which is not exposed in aigverse for good reason.
Let us consider a different approach. Keep exposing mockturtle::cut_view but name it Cut instead of CutAig, and don't make it inherit from Ntk. That way, we can drop all the exception-throwing overloads because on the Python side of things, it won't look like a network at all. It's simply a cut.
I understand that mockturtle has dedicated mockturtle::cut and mockturtle::cut_set classes, but they rely heavily on compile-time parameters. So exposure to Python is non-trivial.
What do you think of this approach? Is there something against it? Would something break? Would this prevent a use case you have in mind?
There was a problem hiding this comment.
I agree, I am thinking of making it simpler rather than a lot of exception-throwing stubs. I'm thinking of supporting indexes directly from the cut (Briefly checked, I think it is doable, since mockturtle::cut_view overrides necessary interfaces) and maybe tell user directly if they would like to perform optimisation operation/statistic analysis on unit cut/cuts, try index to aig method which aigverse already supported.
I'll create another PR
Does this plan sound good to you?
There was a problem hiding this comment.
I'm not entirely sure I understand what your plan is here. In principle, I'm open to a separate PR for easier comparison of the two approaches, so we can pick the one that better suits the overall library design.
However, what do you mean by "I'm thinking of supporting indexes directly from the cut"?
There was a problem hiding this comment.
Similar to a cut-scale version of to_index_list().
There was a problem hiding this comment.
How would that differ from my proposal above?
Let us consider a different approach. Keep exposing
mockturtle::cut_viewbut name itCutinstead ofCutAig, and don't make it inherit fromNtk. That way, we can drop all the exception-throwing overloads because on the Python side of things, it won't look like a network at all. It's simply a cut.
There was a problem hiding this comment.
Your proposal is a first step, but to_index_list() is inherited from Aig if I'm correct, it needs a cut level reconstruction, just an extra step.
There was a problem hiding this comment.
to_index_list() is a member function of Aig. There is no inheritance at work here.
There was a problem hiding this comment.
Yes it is a member function of Aig, I mean if you call that on a cut aig such as cut_aig.to_index_list() it is inherited from Aig silently encode whole network.
What I mean is to prevent such case(It will pass):
def test_cut_aig_to_index_list_encodes_whole_network() -> None:
"""to_index_list() is inherited from Aig and operates on the whole network, not the cut.
Due to mockturtle's non-virtual CRTP dispatch, methods not reimplemented by
cut_view (like to_index_list) silently encode the entire underlying network
instead of just the cut's restricted node set.
"""
aig = Aig()
x1 = aig.create_pi()
x2 = aig.create_pi()
x3 = aig.create_pi()
x4 = aig.create_pi()
f1 = aig.create_and(x1, x2)
f2 = aig.create_and(x3, x4)
f3 = aig.create_and(f1, f2)
aig.create_po(f3)
cut_aig = CutAig(aig, [x1, x2], f1)
assert cut_aig.num_gates == 1
assert cut_aig.num_pis == 2
index_list = cut_aig.to_index_list()
assert index_list.num_gates == aig.num_gates
assert index_list.num_pis == aig.num_pis
There was a problem hiding this comment.
That will already be prevented by my proposal above. If you remove the inheritance from Ntk, CutAig (or Cut, as I would then call it) is no longer a network. It offers some network interface functions, but not enough to be considered one.
| nb::class_<CutNtk, Ntk>(m, fmt::format("Cut{}", network_name).c_str(), | |
| R"pb(Implements an isolated view on a single cut in a network. | |
| This view creates a network from a single cut with a single output `root` | |
| and a set of `leaves`. This is an immutable view; network-modifying methods | |
| are not available. | |
| Note: | |
| This view clears all nodes' visited flags before construction to ensure | |
| the cut is constructed correctly. The view guarantees that all nodes in | |
| the view will have a 0 visited flag after construction.)pb") | |
| nb::class_<CutNtk>(m, "Cut", | |
| R"pb(Implements an isolated view on a single cut in a network. | |
| This view creates a network from a single cut with a single output `root` | |
| and a set of `leaves`. This is an immutable view; network-modifying methods | |
| are not available. | |
| Note: | |
| This view clears all nodes' visited flags before construction to ensure | |
| the cut is constructed correctly. The view guarantees that all nodes in | |
| the view will have a 0 visited flag after construction.)pb") |
2 participants
Description
Expose cut view (and maybe in the future window view) is convenient for developers to do structure level change.
Checklist:
Summary by CodeRabbit