♻️ 💥 Refactor the monolithic package into several extension modules#285
Open
♻️ 💥 Refactor the monolithic package into several extension modules#285
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:
📝 WalkthroughWalkthroughRefactors the Python package into four modular pybind11 extensions (networks, algorithms, io, utils), moves Python sources to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CMake as CMake
participant Cpp as C++_Sources
participant Installer as Installer
participant Py as python/aigverse
participant Import as Python_Importer
Developer->>CMake: configure & call add_aigverse_python_binding(...)
CMake->>Cpp: collect per-module sources (networks, algorithms, io, utils)
CMake->>CMake: build pybind11 modules (networks, algorithms, io, utils)
CMake->>Installer: produce install artifacts (.so / pyd)
Installer->>Py: install shared objects + python package (python/aigverse)
Import->>Py: import aigverse
Py->>Py: lazy-load submodule on access (e.g., aigverse.networks)
Py->>networks: load networks extension module
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python/aigverse/networks.pyi (1)
84-84:⚠️ Potential issue | 🟡 Minor
Aig.__setstate__return type should beNonePython's pickle protocol ignores the return value of
__setstate__; it is expected to beNone. The annotation-> Aigis incorrect and inconsistent withTruthTable.__setstate__inutils.pyi(line 81), which correctly declares-> None.🐛 Proposed fix
- def __setstate__(self, state: tuple[list[int]]) -> Aig: ... + def __setstate__(self, state: tuple[list[int]]) -> None: ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/networks.pyi` at line 84, The type annotation for Aig.__setstate__ is incorrect: change its return type from Aig to None so it matches the pickle protocol and is consistent with TruthTable.__setstate__; update the signature for Aig.__setstate__ in python/aigverse/networks.pyi to return None instead of Aig.docs/aigs.md (2)
208-209:⚠️ Potential issue | 🟡 MinorDuplicate word: "AIGER. AIGER."
📝 Proposed fix
Named AIGs are automatically created when reading from file formats that contain naming information, e.g., Verilog and -AIGER. AIGER. See [File I/O](`#file-io`) for more details. +AIGER. See [File I/O](`#file-io`) for more details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/aigs.md` around lines 208 - 209, The sentence "Named AIGs are automatically created when reading from file formats that contain naming information, e.g., Verilog and AIGER. AIGER." contains a duplicated word "AIGER."; edit the sentence (the line starting with "Named AIGs are automatically created...") to remove the duplicate so it reads with a single "AIGER" (and fix punctuation if needed) ensuring the sentence becomes "..., e.g., Verilog and AIGER.".
16-19:⚠️ Potential issue | 🟡 MinorMultiple narrative cross-references still use the old
aigverse.root path — will produce broken Sphinx links.The root namespace does not re-export these classes; they are only accessible via the
aigverse.networkssubmodule. Update the following cross-references:
Location Current Correct Lines 16, 19, 214 `~aigverse.Aig``~aigverse.networks.Aig`Line 167 `~aigverse.NamedAig``~aigverse.networks.NamedAig`Line 213 `~aigverse.cleanup_dangling``~aigverse.networks.Aig.cleanup_dangling`Line 220 `~aigverse.DepthAig``~aigverse.networks.DepthAig`Line 255 `~aigverse.FanoutAig``~aigverse.networks.FanoutAig`Line 281 `~aigverse.SequentialAig``~aigverse.networks.SequentialAig`Also fix the typo on line 209: "AIGER. AIGER." should be "AIGER."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/aigs.md` around lines 16 - 19, Update the Sphinx cross-references in this document to use the correct submodule path: replace all occurrences of `~aigverse.Aig` with `~aigverse.networks.Aig`, `~aigverse.NamedAig` with `~aigverse.networks.NamedAig`, `~aigverse.cleanup_dangling` with `~aigverse.networks.Aig.cleanup_dangling`, `~aigverse.DepthAig` with `~aigverse.networks.DepthAig`, `~aigverse.FanoutAig` with `~aigverse.networks.FanoutAig`, and `~aigverse.SequentialAig` with `~aigverse.networks.SequentialAig` so the links point to the actual exported classes/methods; also correct the duplicated words typo "AIGER. AIGER." to a single "AIGER." in the same document.
🧹 Nitpick comments (10)
python/aigverse/adapters/networkx.py (3)
148-163:po_idxandself.po_index(po)should yield the same value — consider simplifying.
enumerate(self.pos())already gives the 0-based PO index aspo_idx. The callself.po_index(po)returns the same ordinal for a PO signal. Usingpo_idxdirectly for the synthetic node ID would be simpler and avoid the extra method call.♻️ Suggested simplification
# Iterate over synthetic PO nodes for po_idx, po in enumerate(self.pos()): - synth_node = self.po_index(po) + self.size() + synth_node = po_idx + self.size() attrs = {"index": synth_node}Apply the same at line 193 if adopted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/adapters/networkx.py` around lines 148 - 163, The loop building synthetic PO nodes unnecessarily calls self.po_index(po) even though enumerate(self.pos()) already provides the 0-based PO ordinal; replace the synth_node computation synth_node = self.po_index(po) + self.size() with synth_node = po_idx + self.size() (and update any other uses of self.po_index(po) in that loop to use po_idx) to remove the redundant method call and simplify the logic; apply the same simplification pattern to the analogous PO-creation code near the later block referenced (the code around the other PO loop at the similar location) so both places use po_idx consistently.
169-184: Hoist theAigSignalimport out of the per-edge loop.
from ..networks import AigSignalat line 176 executes on every edge iteration whenhas_namesis True. While Python caches imports, the lookup overhead is unnecessary inside a hot loop. Move it next to thehas_namescheck or to the top of the function.♻️ Suggested refactor
# Iterate over all edges and add them to the graph for src, tgt, weight in [(e.source, e.target, e.weight) for e in edges]: # Assign one-hot encoded edge type based on inversion edge_type = edge_type_inverted if weight else edge_type_regular edge_attrs: dict[str, Any] = {"type": edge_type} # Add signal name if available (edges represent signals) if has_names: - from ..networks import AigSignal - - # source node is an integer (AigNode) - src_int = src - sig = AigSignal(src_int, bool(weight)) + sig = AigSignal(src, bool(weight)) self_named = cast("NamedAig", self) if self_named.has_name(sig): edge_attrs["name"] = self_named.get_name(sig)And add the import near the top of the
if has_namesblock (around line 92) or at function scope:if has_names: from ..networks import AigSignal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/adapters/networkx.py` around lines 169 - 184, The import of AigSignal is inside the per-edge loop causing repeated lookups; move "from ..networks import AigSignal" out of the loop and into the scope where has_names is checked (or to the top of the function) so it is executed once when has_names is True, then keep the rest of the logic that constructs AigSignal(src_int, bool(weight)) and checks self_named.has_name/get_name unchanged — update references to AigSignal, has_names, self_named (cast to NamedAig) accordingly and remove the in-loop import.
91-92: Redundanthasattrguard afterisinstancecheck.At line 122,
hasattr(self, "get_network_name")is alwaysTruewhenhas_namesisTrue(line 92), sinceNamedAigdeclaresget_network_nameper the type stubs. The guard is harmless but unnecessary.Also applies to: 122-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/adapters/networkx.py` around lines 91 - 92, The code uses isinstance(self, NamedAig) to set has_names and then redundantly checks hasattr(self, "get_network_name"); remove the redundant hasattr guard(s) and rely on has_names (or the isinstance check) before calling self.get_network_name() directly; update the checks in the functions referencing has_names/get_network_name (e.g., where has_names is used and the hasattr guard at the later occurrence) so they call self.get_network_name() when has_names is True without the extra hasattr() call.pyproject.toml (1)
138-141: Stale cache-key path:src/**/*.cppshould also coverpython/for Python source changes.The
uvcache key at line 140 referencessrc/**/*.cppfor C++ source changes, which is still correct. However, since the Python package source moved fromsrc/aigversetopython/aigverse, you may want to add a cache key forpython/**/*.pyto invalidate the cache when Python source files change (e.g., the adapters or__init__.pylogic).♻️ Suggested addition
cache-keys = [ { file = "pyproject.toml" }, { git = { commit = true, tags = true } }, { file = "src/**/*.cpp" }, + { file = "python/**/*.py" }, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 138 - 141, The CI cache key list in pyproject.toml currently includes "{ file = "src/**/*.cpp" }" but misses Python sources now located under "python/aigverse"; update the cache key entries (the array that contains the C++ entry) to also include a pattern for Python sources such as "python/**/*.py" so changes to adapters or __init__.py invalidate the cache — locate the cache key array in pyproject.toml and add a new entry mirroring the C++ one but targeting python/**/*.py.python/aigverse/utils.pyi (2)
78-78:__assign__is not a standard Python dunderPython has no
__assign__protocol — assignment rebinds the name and never invokes a method. If pybind11 exposes the C++ copy-assignment operator here, consider renaming it to a regular method (e.g.,assign) to avoid implying special Python semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/utils.pyi` at line 78, The stub exposes a nonstandard dunder method __assign__ on TruthTable which is misleading; change the exposed signature to a normal method name (e.g., assign) and update the typing stub accordingly (replace def __assign__(self, other: TruthTable) -> TruthTable with def assign(self, other: TruthTable) -> TruthTable) so the pybind11 copy-assignment is exposed as a regular method and not a Python dunder.
18-20:ntkattribute type is non-nullable but the constructor acceptsNone
__init__acceptsntk: networks.Aig | None = None, implyingntkmay not be set. The barentk: networks.Aigclass-level annotation would mislead type checkers into treating it as always-present. Aligning the attribute type prevents false "cannot be None" warnings for callers that constructAigEdgeList()without an argument.♻️ Proposed fix
- ntk: networks.Aig + ntk: networks.Aig | None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/utils.pyi` around lines 18 - 20, The class-level attribute annotations for AigEdgeList are non-nullable while its constructor (__init__) allows None; update the attribute types to match the constructor by changing ntk: networks.Aig to ntk: networks.Aig | None and edges: list[AigEdge] to edges: list[AigEdge] | None so type checkers won't complain when AigEdgeList() is constructed without arguments.test/algorithms/test_simulation.py (1)
135-135: DebugLines 135 and 149–150 emit diagnostic output on every test run. These are noise in CI logs and should be removed before merge.
♻️ Proposed fix
- print(f"MAJ: {majority.to_binary()}") - assert len(sim) == 1- for i in range(len(n_map)): - print(f"Node {i}: {n_map[i].to_binary()}") - assert len(n_map) == 8Also applies to: 149-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/algorithms/test_simulation.py` at line 135, Remove the leftover diagnostic print statements in the test (specifically the print(f"MAJ: {majority.to_binary()}") and the prints around the later assertions at the end of test_simulation.py) so tests do not emit noise in CI; locate the prints by searching for majority.to_binary() and any plain print(...) calls in test_simulation.py and delete them (or replace with logger.debug(...) if persistent debug output is required).test/test_import_contract.py (2)
24-32:test_lazy_submodule_loadingmutatessys.moduleswithout guaranteed cleanup.If
_ = aigverse.networks(line 31) raises — e.g., if the lazy-loader__getattr__is missing or broken —aigverse.networksstays absent fromsys.modulesfor all subsequent tests in the session, producing confusing failures unrelated to those tests.Use
monkeypatch(pytest's built-in fixture) which restores the original state on teardown regardless of pass/fail:♻️ Proposed fix using monkeypatch
-def test_lazy_submodule_loading() -> None: - import aigverse - - sys.modules.pop("aigverse.networks", None) - aigverse.__dict__.pop("networks", None) - - assert "aigverse.networks" not in sys.modules - _ = aigverse.networks - assert "aigverse.networks" in sys.modules +def test_lazy_submodule_loading(monkeypatch) -> None: + import aigverse + + monkeypatch.delitem(sys.modules, "aigverse.networks", raising=False) + monkeypatch.delitem(aigverse.__dict__, "networks", raising=False) + + assert "aigverse.networks" not in sys.modules + _ = aigverse.networks + assert "aigverse.networks" in sys.modules🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_import_contract.py` around lines 24 - 32, The test test_lazy_submodule_loading mutates sys.modules directly and can leave global state broken if aigverse.networks import fails; update the test to use pytest's monkeypatch fixture to safely manipulate sys.modules and aigverse.__dict__ (e.g., monkeypatch.dict(sys.modules, {...}) and monkeypatch.delitem or monkeypatch.setitem for aigverse.__dict__) so the original entries are restored on teardown even if accessing aigverse.networks raises; refer to the test function name test_lazy_submodule_loading and the symbols sys.modules and aigverse.__dict__ when making the change.
14-16:assert Aig is not Noneis vacuously true for any class.A class object imported from a C extension can never be
None. Replace with a more meaningful assertion.♻️ Proposed fix
- assert Aig is not None + assert isinstance(Aig, type)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_import_contract.py` around lines 14 - 16, The current test uses "assert Aig is not None" which is meaningless for an imported class; replace it with a meaningful assertion about the Aig symbol: for example, verify Aig is callable and can be instantiated (call Aig() or Aig(default_args) as appropriate) and then assert the result is an instance of Aig (isinstance(instance, Aig)) or check a known attribute/method exists on the class/instance (e.g., hasattr(Aig, "forward") or hasattr(instance, "predict")). Update the test to create a real instance of Aig and assert on its type or behavior rather than asserting Aig is not None.cmake/AddAigversePythonBinding.cmake (1)
2-3: Prefercmake_parse_arguments(PARSE_ARGV 1 ...)over${ARGN}expansion.The
${ARGN}form is susceptible to list flattening — filenames or values containing semicolons will be silently split into multiple tokens.PARSE_ARGV 1(CMake ≥ 3.7, well within this project's minimum) avoids this and is the idiomatic modern form.♻️ Suggested change
function(add_aigverse_python_binding target_name) - cmake_parse_arguments(ARG "" "MODULE_NAME;INSTALL_DIR" "" ${ARGN}) - set(SOURCES ${ARG_UNPARSED_ARGUMENTS}) + cmake_parse_arguments(PARSE_ARGV 1 ARG "" "MODULE_NAME;INSTALL_DIR" "") + set(SOURCES ${ARG_UNPARSED_ARGUMENTS})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/AddAigversePythonBinding.cmake` around lines 2 - 3, Replace the vulnerable ${ARGN} expansion with the PARSE_ARGV form: call cmake_parse_arguments using the PARSE_ARGV 1 mode (i.e., cmake_parse_arguments(ARG "" "MODULE_NAME;INSTALL_DIR" "" PARSE_ARGV 1)) so arguments containing semicolons are not flattened, and continue to populate/set SOURCES from ARG_UNPARSED_ARGUMENTS (set(SOURCES ${ARG_UNPARSED_ARGUMENTS})). Ensure you remove the ${ARGN} usage and use cmake_parse_arguments(ARG ... PARSE_ARGV 1) to parse the caller's argv correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/algorithms.md`:
- Line 24: Update the stale Sphinx cross-references for the functions simulate
and simulate_nodes so they point to the functions' current module paths instead
of the old root export (replace `{py:func}`~aigverse.simulate`` and
`{py:func}`~aigverse.simulate_nodes`` with the functions' correct
fully-qualified module names where simulate and simulate_nodes are actually
defined), then rebuild docs to confirm Sphinx resolves the references without
warnings.
In `@python/aigverse/io.pyi`:
- Line 3: Change the incorrect two-dot relative imports to single-dot imports
for the C++ extension stubs: replace "from .. import networks" in the import
statement shown (symbol: the import line "from .. import networks" in
aigverse.io.pyi) with "from . import networks", and make the analogous fixes in
the other stub files by changing the import lines referencing algorithms and
utils from "from .. import algorithms" and "from .. import utils" to "from .
import algorithms" and "from . import utils" so the stubs resolve as
aigverse.networks, aigverse.algorithms, and aigverse.utils.
In `@test/inout/test_write_aiger.py`:
- Line 6: The inline comment "# Get the temporary directory as a Path object" is
misplaced on the import line with read_aiger_into_aig and write_aiger; remove
that comment from the import statement and move it to the temp_dir assignment
(the variable temp_dir = Path(tempfile.gettempdir())) so the comment accurately
describes the Path conversion. Ensure the import line only contains the import
and the temp_dir line has the explanatory comment.
In `@test/inout/test_write_dot.py`:
- Line 6: The inline comment after the import is misplaced: remove the trailing
comment from the write_dot import line (the symbol write_dot in the test) and
either delete it or move it so it sits above or inline with the temp_dir
assignment (the temp_dir variable on line where the temporary directory is
created) so the comment correctly documents temp_dir instead of the write_dot
import. Ensure no other imports are annotated with that comment.
In `@test/inout/test_write_verilog.py`:
- Line 6: Move the misplaced inline comment from the import line to the temp_dir
assignment: remove the comment "# Get the temporary directory as a Path object"
from the import statement that contains read_verilog_into_aig and write_verilog,
and add that comment to the temp_dir variable declaration (the line where
temp_dir is assigned) so the comment correctly documents temp_dir instead of the
import.
In `@UPGRADING.md`:
- Around line 15-16: Add migration notes for the moved/renamed functions: state
that cleanup_dangling is now available as aigverse.utils.cleanup_dangling and
that to_aig is now exported from aigverse.utils (or show the new import pattern
`from aigverse.utils import cleanup_dangling, to_aig`) and update any examples
referencing `from aigverse import cleanup_dangling` to use the new import; also
mention any behavioral or signature changes if applicable and ensure
docs/aigs.md references to `{py:func}`~aigverse.cleanup_dangling`` point to the
new location `aigverse.utils.cleanup_dangling`.
---
Outside diff comments:
In `@docs/aigs.md`:
- Around line 208-209: The sentence "Named AIGs are automatically created when
reading from file formats that contain naming information, e.g., Verilog and
AIGER. AIGER." contains a duplicated word "AIGER."; edit the sentence (the line
starting with "Named AIGs are automatically created...") to remove the duplicate
so it reads with a single "AIGER" (and fix punctuation if needed) ensuring the
sentence becomes "..., e.g., Verilog and AIGER.".
- Around line 16-19: Update the Sphinx cross-references in this document to use
the correct submodule path: replace all occurrences of `~aigverse.Aig` with
`~aigverse.networks.Aig`, `~aigverse.NamedAig` with
`~aigverse.networks.NamedAig`, `~aigverse.cleanup_dangling` with
`~aigverse.networks.Aig.cleanup_dangling`, `~aigverse.DepthAig` with
`~aigverse.networks.DepthAig`, `~aigverse.FanoutAig` with
`~aigverse.networks.FanoutAig`, and `~aigverse.SequentialAig` with
`~aigverse.networks.SequentialAig` so the links point to the actual exported
classes/methods; also correct the duplicated words typo "AIGER. AIGER." to a
single "AIGER." in the same document.
In `@python/aigverse/networks.pyi`:
- Line 84: The type annotation for Aig.__setstate__ is incorrect: change its
return type from Aig to None so it matches the pickle protocol and is consistent
with TruthTable.__setstate__; update the signature for Aig.__setstate__ in
python/aigverse/networks.pyi to return None instead of Aig.
---
Duplicate comments:
In `@python/aigverse/algorithms.pyi`:
- Line 3: The relative import uses one level too many; in
python/aigverse/algorithms.pyi replace "from .. import networks, utils" with a
same-package import (e.g., "from . import networks, utils") so it correctly
imports the sibling modules networks and utils; update the import in
algorithms.pyi (referencing the symbols networks and utils) to match the pattern
used in io.pyi and utils.pyi.
In `@python/aigverse/utils.pyi`:
- Line 4: The relative import depth is incorrect: replace the problematic import
statement "from .. import networks" in utils.pyi with the correct import for the
package layout (either adjust the relative depth, e.g. "from . import networks"
or the proper absolute import like "from aigverse import networks") so it
matches the package structure used elsewhere (same fix applied to io.pyi);
update the single import line so type stubs resolve correctly.
---
Nitpick comments:
In `@cmake/AddAigversePythonBinding.cmake`:
- Around line 2-3: Replace the vulnerable ${ARGN} expansion with the PARSE_ARGV
form: call cmake_parse_arguments using the PARSE_ARGV 1 mode (i.e.,
cmake_parse_arguments(ARG "" "MODULE_NAME;INSTALL_DIR" "" PARSE_ARGV 1)) so
arguments containing semicolons are not flattened, and continue to populate/set
SOURCES from ARG_UNPARSED_ARGUMENTS (set(SOURCES ${ARG_UNPARSED_ARGUMENTS})).
Ensure you remove the ${ARGN} usage and use cmake_parse_arguments(ARG ...
PARSE_ARGV 1) to parse the caller's argv correctly.
In `@pyproject.toml`:
- Around line 138-141: The CI cache key list in pyproject.toml currently
includes "{ file = "src/**/*.cpp" }" but misses Python sources now located under
"python/aigverse"; update the cache key entries (the array that contains the C++
entry) to also include a pattern for Python sources such as "python/**/*.py" so
changes to adapters or __init__.py invalidate the cache — locate the cache key
array in pyproject.toml and add a new entry mirroring the C++ one but targeting
python/**/*.py.
In `@python/aigverse/adapters/networkx.py`:
- Around line 148-163: The loop building synthetic PO nodes unnecessarily calls
self.po_index(po) even though enumerate(self.pos()) already provides the 0-based
PO ordinal; replace the synth_node computation synth_node = self.po_index(po) +
self.size() with synth_node = po_idx + self.size() (and update any other uses of
self.po_index(po) in that loop to use po_idx) to remove the redundant method
call and simplify the logic; apply the same simplification pattern to the
analogous PO-creation code near the later block referenced (the code around the
other PO loop at the similar location) so both places use po_idx consistently.
- Around line 169-184: The import of AigSignal is inside the per-edge loop
causing repeated lookups; move "from ..networks import AigSignal" out of the
loop and into the scope where has_names is checked (or to the top of the
function) so it is executed once when has_names is True, then keep the rest of
the logic that constructs AigSignal(src_int, bool(weight)) and checks
self_named.has_name/get_name unchanged — update references to AigSignal,
has_names, self_named (cast to NamedAig) accordingly and remove the in-loop
import.
- Around line 91-92: The code uses isinstance(self, NamedAig) to set has_names
and then redundantly checks hasattr(self, "get_network_name"); remove the
redundant hasattr guard(s) and rely on has_names (or the isinstance check)
before calling self.get_network_name() directly; update the checks in the
functions referencing has_names/get_network_name (e.g., where has_names is used
and the hasattr guard at the later occurrence) so they call
self.get_network_name() when has_names is True without the extra hasattr() call.
In `@python/aigverse/utils.pyi`:
- Line 78: The stub exposes a nonstandard dunder method __assign__ on TruthTable
which is misleading; change the exposed signature to a normal method name (e.g.,
assign) and update the typing stub accordingly (replace def __assign__(self,
other: TruthTable) -> TruthTable with def assign(self, other: TruthTable) ->
TruthTable) so the pybind11 copy-assignment is exposed as a regular method and
not a Python dunder.
- Around line 18-20: The class-level attribute annotations for AigEdgeList are
non-nullable while its constructor (__init__) allows None; update the attribute
types to match the constructor by changing ntk: networks.Aig to ntk:
networks.Aig | None and edges: list[AigEdge] to edges: list[AigEdge] | None so
type checkers won't complain when AigEdgeList() is constructed without
arguments.
In `@test/algorithms/test_simulation.py`:
- Line 135: Remove the leftover diagnostic print statements in the test
(specifically the print(f"MAJ: {majority.to_binary()}") and the prints around
the later assertions at the end of test_simulation.py) so tests do not emit
noise in CI; locate the prints by searching for majority.to_binary() and any
plain print(...) calls in test_simulation.py and delete them (or replace with
logger.debug(...) if persistent debug output is required).
In `@test/test_import_contract.py`:
- Around line 24-32: The test test_lazy_submodule_loading mutates sys.modules
directly and can leave global state broken if aigverse.networks import fails;
update the test to use pytest's monkeypatch fixture to safely manipulate
sys.modules and aigverse.__dict__ (e.g., monkeypatch.dict(sys.modules, {...})
and monkeypatch.delitem or monkeypatch.setitem for aigverse.__dict__) so the
original entries are restored on teardown even if accessing aigverse.networks
raises; refer to the test function name test_lazy_submodule_loading and the
symbols sys.modules and aigverse.__dict__ when making the change.
- Around line 14-16: The current test uses "assert Aig is not None" which is
meaningless for an imported class; replace it with a meaningful assertion about
the Aig symbol: for example, verify Aig is callable and can be instantiated
(call Aig() or Aig(default_args) as appropriate) and then assert the result is
an instance of Aig (isinstance(instance, Aig)) or check a known attribute/method
exists on the class/instance (e.g., hasattr(Aig, "forward") or hasattr(instance,
"predict")). Update the test to create a real instance of Aig and assert on its
type or behavior rather than asserting Aig is not None.
UPGRADING.md
Outdated
Comment on lines
15
to
16
| - **Utils**: `TruthTable`, `AigEdge`, `AigEdgeList`, `AigIndexList` and helper functions like `to_edge_list`, | ||
| `to_index_list` are now in `aigverse.utils`. |
There was a problem hiding this comment.
cleanup_dangling and to_aig are absent from the migration guide.
to_aig is listed as a aigverse.utils export in docs/aigs.md (line 373) but is not mentioned here. More critically, cleanup_dangling — referenced in docs/aigs.md as {py:func}~aigverse.cleanup_dangling`` — is not mentioned at all. Users upgrading from from aigverse import cleanup_dangling have no documented migration path.
📝 Suggested additions
- **Utils**: `TruthTable`, `AigEdge`, `AigEdgeList`, `AigIndexList` and helper functions like `to_edge_list`,
- `to_index_list` are now in `aigverse.utils`.
+ `to_index_list`, `to_aig` are now in `aigverse.utils`.
+- **Algorithms**: `cleanup_dangling` is now in `aigverse.algorithms`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Utils**: `TruthTable`, `AigEdge`, `AigEdgeList`, `AigIndexList` and helper functions like `to_edge_list`, | |
| `to_index_list` are now in `aigverse.utils`. | |
| - **Utils**: `TruthTable`, `AigEdge`, `AigEdgeList`, `AigIndexList` and helper functions like `to_edge_list`, | |
| `to_index_list`, `to_aig` are now in `aigverse.utils`. | |
| - **Algorithms**: `cleanup_dangling` is now in `aigverse.algorithms`. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@UPGRADING.md` around lines 15 - 16, Add migration notes for the moved/renamed
functions: state that cleanup_dangling is now available as
aigverse.utils.cleanup_dangling and that to_aig is now exported from
aigverse.utils (or show the new import pattern `from aigverse.utils import
cleanup_dangling, to_aig`) and update any examples referencing `from aigverse
import cleanup_dangling` to use the new import; also mention any behavioral or
signature changes if applicable and ensure docs/aigs.md references to
`{py:func}`~aigverse.cleanup_dangling`` point to the new location
`aigverse.utils.cleanup_dangling`.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aigverse/utils/bindings.cpp (1)
7-8: Consider splitting network adapters out ofutilsinto a dedicatedaigverse.adapterssubmodule.
bind_to_edge_listandbind_to_index_listoriginate fromsrc/aigverse/adapters/and were listed as a distinct "Adapters" section in the legacyaigverse.cpp. Bundling them underutils(whose doc reads "Utility data structures and functions") conflates format-conversion adapters with data-structure utilities. A user looking to convert a network to an edge list won't necessarily look inaigverse.utils.♻️ Optional: introduce a thin
src/aigverse/adapters/bindings.cpp// src/aigverse/adapters/bindings.cpp (new file) +#include <pybind11/pybind11.h> + +namespace aigverse +{ +void bind_to_edge_list(pybind11::module_& m); +void bind_to_index_list(pybind11::module_& m); +} // namespace aigverse + +PYBIND11_MODULE(adapters, m, pybind11::mod_gil_not_used()) // NOLINT(misc-include-cleaner) +{ + m.doc() = "Network adapter utilities"; + aigverse::bind_to_edge_list(m); + aigverse::bind_to_index_list(m); +}// src/aigverse/utils/bindings.cpp namespace aigverse { void bind_truth_table(pybind11::module_& m); void bind_truth_table_operations(pybind11::module_& m); -void bind_to_edge_list(pybind11::module_& m); -void bind_to_index_list(pybind11::module_& m); } // namespace aigverse PYBIND11_MODULE(utils, m, pybind11::mod_gil_not_used()) { m.doc() = "Utility data structures and functions"; aigverse::bind_truth_table(m); aigverse::bind_truth_table_operations(m); - aigverse::bind_to_edge_list(m); - aigverse::bind_to_index_list(m); }Also applies to: 16-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aigverse/utils/bindings.cpp` around lines 7 - 8, The two adapter binding declarations bind_to_edge_list and bind_to_index_list belong in an adapters submodule, not utils; move their declarations/definitions from src/aigverse/utils/bindings.cpp into a new src/aigverse/adapters/bindings.cpp, create/initialize a Python submodule (aigverse.adapters) in the top-level module init and register the functions there (call bind_to_edge_list(aigverse.adapters) and bind_to_index_list(aigverse.adapters)), and remove or update the utils bindings to no longer export these adapter functions so the adapters live under aigverse.adapters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/aigverse/utils/bindings.cpp`:
- Around line 7-8: The two adapter binding declarations bind_to_edge_list and
bind_to_index_list belong in an adapters submodule, not utils; move their
declarations/definitions from src/aigverse/utils/bindings.cpp into a new
src/aigverse/adapters/bindings.cpp, create/initialize a Python submodule
(aigverse.adapters) in the top-level module init and register the functions
there (call bind_to_edge_list(aigverse.adapters) and
bind_to_index_list(aigverse.adapters)), and remove or update the utils bindings
to no longer export these adapter functions so the adapters live under
aigverse.adapters.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
UPGRADING.md (1)
23-43: Before/After example includes items not present in the "Before" block.The "After" block introduces
to_aig(line 37) andaig.cleanup_dangling()(line 40) that have no counterpart in the "Before" block. This blurs the line between "migration change" and "new feature". Consider either adding the old equivalents in the "Before" block or removing them from "After" to keep the migration example focused on import path changes only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UPGRADING.md` around lines 23 - 43, The After example introduces to_aig and aig.cleanup_dangling() which aren't in the Before example; update the migration snippet to only show import path changes by either (a) adding the old equivalents (to_aig and whatever produced/cleaned dangling nodes) to the Before block so both blocks show the same operations (referencing Aig, read_aiger_into_aig, balancing, TruthTable, to_aig, cleanup_dangling) or (b) remove to_aig and aig.cleanup_dangling() from the After block so it only demonstrates the import/module path changes for Aig, read_aiger_into_aig, balancing, and TruthTable.pyproject.toml (1)
120-120: Consider expanding the cibuildwheel smoke test to cover all submodules.The current test only imports from
aigverse.networks. If any of the other three extension modules (algorithms,io,utils) fail to build or install correctly, this smoke test won't catch it.♻️ Suggested improvement
-test-command = "python -c \"from aigverse.networks import Aig\"" +test-command = "python -c \"from aigverse.networks import Aig; from aigverse.algorithms import simulate; from aigverse.io import read_aiger_into_aig; from aigverse.utils import TruthTable\""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 120, The current cibuildwheel test-command only imports Aig from aigverse.networks; update the test-command to import all four extension submodules so build/install failures in any of them are detected — change the test-command entry (test-command) to run a short python import that touches aigverse.networks, aigverse.algorithms, aigverse.io, and aigverse.utils (for example: a one-liner import statement that imports those module names) so the smoke test validates all submodules.python/aigverse/adapters/networkx.py (1)
91-95:self_namedtype is never narrowed toNamedAig;AigSignalis potentially unbound under static analysis.
self_namedhas typeNamedAig | None. Mypy does not narrow it through thehas_names: boolflag — it only narrows via directisinstanceoris not Nonechecks. Everyself_named.<attr>call inside anyif has_names:block (lines 125, 189–194, 182–183) will produce "Item 'None' of 'NamedAig | None' has no attribute …" errors. Similarly,AigSignalis defined inside oneif has_names:branch (line 94) and used inside a differentif has_names:branch (line 181), which mypy/pyright may flag as "possibly unbound."The project already uses mypy (line 193 has a
# type: ignore), so these would surface as real CI failures.Cleaner pattern: use a single
isinstanceguard that naturally narrows bothselfandself_named, and importAigSignalat module level alongsideNamedAig:♻️ Suggested refactor
In the module-level imports (line 11):
-from ..networks import DepthAig, NamedAig +from ..networks import AigSignal, DepthAig, NamedAigInside
to_networkx, replace lines 91–95:- # Check if this is a NamedAig - has_names = isinstance(self, NamedAig) - self_named = cast("NamedAig", self) if has_names else None - if has_names: - from ..networks import AigSignal + # Use a direct reference so type narrowing works throughout + self_named: NamedAig | None = self if isinstance(self, NamedAig) else NoneThen replace every
if has_names:guard that accessesself_namedwithif self_named is not None:. This gives mypy the narrowing it needs and removes the conditional import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/adapters/networkx.py` around lines 91 - 95, The code creates has_names/self_named and conditionally imports AigSignal inside to_networkx, which prevents static type narrowing (self_named remains NamedAig | None and AigSignal can be seen as unbound); fix by moving AigSignal import to the module-level imports alongside NamedAig and replace the has_names pattern in to_networkx with a direct isinstance check that narrows self (e.g., if isinstance(self, NamedAig): self_named = cast("NamedAig", self) ...), then use if self_named is not None: (or the same isinstance block) wherever self_named attributes are accessed (lines referencing self_named and uses of AigSignal), so mypy/pyright can correctly narrow types and remove the conditional import.python/aigverse/utils.pyi (1)
78-78:__assign__is not a Python protocol — consider a docstring or renaming.Python has no
__assign__dunder;tb1 = tb2rebinds the Python name rather than calling this method. This is a pybind11 artefact from bindingoperator=. Users expecting value-copy semantics via=will be surprised. Consider exposing this ascopy_from(other)or a@classmethodfactory, and/or adding a comment in the stub so callers understand the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/utils.pyi` at line 78, The stub exposes a pybind11-generated C++ operator= as def __assign__(self, other: TruthTable) -> TruthTable which is misleading because Python name binding does not call this dunder; rename or replace it with a clear Pythonic API (e.g., instance method copy_from(self, other: TruthTable) -> TruthTable and/or a `@classmethod` factory like from_other(cls, other: TruthTable) -> TruthTable) and add a docstring on the TruthTable type explaining that this was the C++ assignment operator bound by pybind11 and that Python assignment will not invoke it; update the stub signature(s) and docstring accordingly to avoid confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/aigverse/utils.pyi`:
- Around line 17-20: The AigEdgeList __init__ in the stub incorrectly allows ntk
and edges to be None; replace that single signature with three `@overload`
variants to match the C++ bindings: a no-arg __init__, an __init__(ntk:
networks.Aig), and an __init__(ntk: networks.Aig, edges: list[AigEdge]) so
callers cannot pass None; keep the attributes ntk: networks.Aig and edges:
list[AigEdge] as non-Optional and import/use typing.overload for the overloads.
---
Duplicate comments:
In `@python/aigverse/io.pyi`:
- Line 3: The relative import in python/aigverse/io.pyi has been corrected to
"from . import networks"; no further code changes are needed—keep this
single-dot relative import as-is and proceed to approve the change.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 120: The current cibuildwheel test-command only imports Aig from
aigverse.networks; update the test-command to import all four extension
submodules so build/install failures in any of them are detected — change the
test-command entry (test-command) to run a short python import that touches
aigverse.networks, aigverse.algorithms, aigverse.io, and aigverse.utils (for
example: a one-liner import statement that imports those module names) so the
smoke test validates all submodules.
In `@python/aigverse/adapters/networkx.py`:
- Around line 91-95: The code creates has_names/self_named and conditionally
imports AigSignal inside to_networkx, which prevents static type narrowing
(self_named remains NamedAig | None and AigSignal can be seen as unbound); fix
by moving AigSignal import to the module-level imports alongside NamedAig and
replace the has_names pattern in to_networkx with a direct isinstance check that
narrows self (e.g., if isinstance(self, NamedAig): self_named = cast("NamedAig",
self) ...), then use if self_named is not None: (or the same isinstance block)
wherever self_named attributes are accessed (lines referencing self_named and
uses of AigSignal), so mypy/pyright can correctly narrow types and remove the
conditional import.
In `@python/aigverse/utils.pyi`:
- Line 78: The stub exposes a pybind11-generated C++ operator= as def
__assign__(self, other: TruthTable) -> TruthTable which is misleading because
Python name binding does not call this dunder; rename or replace it with a clear
Pythonic API (e.g., instance method copy_from(self, other: TruthTable) ->
TruthTable and/or a `@classmethod` factory like from_other(cls, other: TruthTable)
-> TruthTable) and add a docstring on the TruthTable type explaining that this
was the C++ assignment operator bound by pybind11 and that Python assignment
will not invoke it; update the stub signature(s) and docstring accordingly to
avoid confusion.
In `@UPGRADING.md`:
- Around line 23-43: The After example introduces to_aig and
aig.cleanup_dangling() which aren't in the Before example; update the migration
snippet to only show import path changes by either (a) adding the old
equivalents (to_aig and whatever produced/cleaned dangling nodes) to the Before
block so both blocks show the same operations (referencing Aig,
read_aiger_into_aig, balancing, TruthTable, to_aig, cleanup_dangling) or (b)
remove to_aig and aig.cleanup_dangling() from the After block so it only
demonstrates the import/module path changes for Aig, read_aiger_into_aig,
balancing, and TruthTable.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/conf.py (1)
109-109:autoapi_dirspath update is correct.
../python/aigverseresolves correctly fromdocs/conf.pyto the new package root. The existingautoapi_python_use_implicit_namespaces = Trueis the right setting for the lazy-loading package layout.One pre-existing note: the
autoapi_ignorepattern*/**/_version.py(line 114) requires at least one directory component before_version.py, so it may not suppress the root-levelpython/aigverse/_version.pythat is now the top of the scanned directory tree. Consider adding"_version.py"or"**/_version.py"as an additional ignore entry if autoapi tries to document the auto-generated version file.🛡️ Proposed guard for root-level
_version.pyautoapi_ignore = [ "*/**/_version.py", + "_version.py", "*/**/test/*", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/conf.py` at line 109, The autoapi ignore pattern may not catch a root-level python/aigverse/_version.py; update the Sphinx config by adding an explicit ignore for the root-level version file (e.g., add "_version.py" or "**/_version.py") to the autoapi_ignore list so autoapi_dirs = ["../python/aigverse"] doesn't cause the generated _version.py to be documented; modify the autoapi_ignore variable in docs/conf.py (alongside existing autoapi_python_use_implicit_namespaces) to include the additional pattern.pyproject.toml (1)
120-120: Smoke test covers only one of four extension modules.
from aigverse.networks import Aigwon't detect build failures inaigverse-algorithms,aigverse-io, oraigverse-utils. Consider exercising at least one representative import from each submodule.🔍 Expanded smoke test
-test-command = "python -c \"from aigverse.networks import Aig\"" +test-command = "python -c \"from aigverse.networks import Aig; from aigverse.algorithms import balance; from aigverse.io import write_aiger; from aigverse.utils import TruthTable\""(Adjust the specific symbols to match what is actually exported by each submodule.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 120, The current smoke test only imports Aig from aigverse.networks and misses other packages; update the pyproject.toml test-command to import at least one representative public symbol from each subpackage (e.g., a symbol from aigverse.algorithms, aigverse.io, and aigverse.utils in addition to aigverse.networks) so build/import errors in those packages are detected; modify the test-command to run a single python -c that imports one exported name from each module (replace the placeholder symbols with the actual exported names from aigverse.algorithms, aigverse.io, and aigverse.utils).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/conf.py`:
- Line 109: The autoapi ignore pattern may not catch a root-level
python/aigverse/_version.py; update the Sphinx config by adding an explicit
ignore for the root-level version file (e.g., add "_version.py" or
"**/_version.py") to the autoapi_ignore list so autoapi_dirs =
["../python/aigverse"] doesn't cause the generated _version.py to be documented;
modify the autoapi_ignore variable in docs/conf.py (alongside existing
autoapi_python_use_implicit_namespaces) to include the additional pattern.
In `@pyproject.toml`:
- Line 120: The current smoke test only imports Aig from aigverse.networks and
misses other packages; update the pyproject.toml test-command to import at least
one representative public symbol from each subpackage (e.g., a symbol from
aigverse.algorithms, aigverse.io, and aigverse.utils in addition to
aigverse.networks) so build/import errors in those packages are detected; modify
the test-command to run a single python -c that imports one exported name from
each module (replace the placeholder symbols with the actual exported names from
aigverse.algorithms, aigverse.io, and aigverse.utils).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
UPGRADING.md (1)
14-14: IO section omits specific function names, unlike every other section.All other bullets list the symbols that moved (e.g.,
Aig,balancing,TruthTable). The IO bullet only says "Reading/Writing functions", leaving users to guess which functions are affected. Listing at least the most-used functions (e.g.,read_aiger_into_aig,write_aiger) would make this consistent and easier to grep against existing codebases.📝 Suggested wording
-- **IO**: Reading/Writing functions are now in `aigverse.io`. +- **IO**: `read_aiger_into_aig`, `write_aiger`, and other reading/writing functions are now in `aigverse.io`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UPGRADING.md` at line 14, The IO bullet in UPGRADING.md is vague and should enumerate the moved function symbols; update the line about "Reading/Writing functions are now in `aigverse.io`" to list the common functions that changed (for example `read_aiger_into_aig`, `write_aiger`, and other frequently used readers/writers), so users can grep and update imports; reference `aigverse.io` as the new module and include the specific function names moved from their previous locations.src/aigverse/algorithms/bindings.cpp (1)
3-11: Consider extracting forward declarations into a shared header for compile-time safety.The six
bind_*forward declarations are co-located in the.cppfile that uses them. A signature drift in any definition file (e.g.,bind_balancing.cpp) will only surface as a linker error, not a compiler diagnostic. Extracting them into a dedicatedalgorithms_bindings.hppheader and including it in both the definition files and here would catch mismatches at compile time.♻️ Suggested refactor
Create
src/aigverse/algorithms/algorithms_bindings.hpp:`#pragma` once `#include` <pybind11/pybind11.h> namespace aigverse { void bind_equivalence_checking(pybind11::module_& m); void bind_refactoring(pybind11::module_& m); void bind_resubstitution(pybind11::module_& m); void bind_rewriting(pybind11::module_& m); void bind_balancing(pybind11::module_& m); void bind_simulation(pybind11::module_& m); } // namespace aigverseThen in
bindings.cpp:`#include` <pybind11/pybind11.h> +#include "algorithms_bindings.hpp" -namespace aigverse -{ -void bind_equivalence_checking(pybind11::module_& m); -void bind_refactoring(pybind11::module_& m); -void bind_resubstitution(pybind11::module_& m); -void bind_rewriting(pybind11::module_& m); -void bind_balancing(pybind11::module_& m); -void bind_simulation(pybind11::module_& m); -} // namespace aigverse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aigverse/algorithms/bindings.cpp` around lines 3 - 11, The six bind_* forward declarations (bind_equivalence_checking, bind_refactoring, bind_resubstitution, bind_rewriting, bind_balancing, bind_simulation) live only in bindings.cpp which risks signature drift; create a new header (e.g., algorithms_bindings.hpp) that declares these functions taking pybind11::module_& and include that header both in this bindings.cpp and in each corresponding definition file (e.g., bind_balancing.cpp) so mismatched signatures are caught at compile time; update includes accordingly and remove the local forward declarations from bindings.cpp.python/aigverse/adapters/networkx.py (2)
175-179:src_int = srcis a redundant assignment.
srcfrom the edge-list unpacking is already an integer (AigNode). The intermediate bindingsrc_intadds no clarity and is immediately consumed on the next line.♻️ Proposed simplification
if self_named is not None: - # source node is an integer (AigNode) - src_int = src - sig = AigSignal(src_int, bool(weight)) + sig = AigSignal(src, bool(weight)) if self_named.has_name(sig): edge_attrs["name"] = self_named.get_name(sig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/adapters/networkx.py` around lines 175 - 179, Remove the redundant local binding src_int in the block where you handle integer source nodes: instead of assigning src_int = src then using src_int, construct the AigSignal directly from src (e.g., AigSignal(src, bool(weight))) and use that signal for the subsequent self_named.has_name check; update the references to use src and the newly created signal variable and delete the unnecessary src_int declaration.
91-92:castis redundant afterisinstancenarrowing.Once
isinstance(self, NamedAig)isTrue, a type checker already narrows the type toNamedAigin that branch, makingcast("NamedAig", self)a no-op from a static-analysis perspective. The only reason to keep it is as a pybind11 workaround if the type checker fails to narrow pybind11 extension types — in which case a brief comment explaining why would be helpful.♻️ Proposed simplification (if narrowing works correctly)
- self_named = cast("NamedAig", self) if isinstance(self, NamedAig) else None + self_named: NamedAig | None = self if isinstance(self, NamedAig) else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/aigverse/adapters/networkx.py` around lines 91 - 92, The cast("NamedAig", self) after isinstance(self, NamedAig) is redundant; either remove the cast and use the narrowed type directly (replace self_named = cast("NamedAig", self) if isinstance(self, NamedAig) else None with a simple conditional assignment using the narrowed self), or if this is a pybind11 extension type that the type checker doesn't narrow, keep the cast but add a one-line comment explaining the pybind11 narrowing workaround; update occurrences around NamedAig, isinstance, and self_named in python/aigverse/adapters/networkx.py accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/aigverse/adapters/networkx.py`:
- Around line 174-191: The edge attribute "name" assigned in the edge creation
(around g.add_edge where AigSignal(sig) and self_named.has_name(sig) are used)
is later unconditionally overwritten by the PO output assignment in the loop
over self_named.pos(); change the attribute scheme to preserve both values: when
setting the signal name during edge creation, store it under a distinct key
(e.g., "signal_name") instead of "name" and when assigning PO output names to
edges entering synthetic PO nodes, store them under a distinct key (e.g.,
"output_name") (update the g.add_edge block and the g.edges[pred,
synthetic_po_node] assignment accordingly) so the signal name is not silently
lost; adjust any downstream consumers if they expect the old "name" key.
- Around line 148-163: Remove the redundant src_int assignment and use src
directly when creating edges (the variables involved are src from the edge tuple
and the synth_node computed as po_idx + self.size()); additionally, when
overwriting edge attributes for synthetic PO nodes (the code that sets
edge["name"] to PO output names for edges targeting synth_node), preserve any
existing signal name as a fallback—only replace edge["name"] if the PO output
name is non-empty/defined (or use a setdefault-style behavior) so the original
signal name set earlier is not silently lost; update the logic in the loops that
build edges and that post-process PO-targeting edges (references: pos(), size(),
po_idx, synth_node, edge["name"], graph_funcs) accordingly.
In `@UPGRADING.md`:
- Line 17: Add an explicit before/after calling example to the UPGRADING.md
entry for cleanup_dangling so users see the new instance method syntax; replace
or augment the current line mentioning aigverse.networks.Aig.cleanup_dangling
with a short Python fenced snippet showing the old free-function call (from
aigverse import cleanup_dangling; cleanup_dangling(aig)) and the new method call
(aig.cleanup_dangling()), and ensure the example references the free function
name cleanup_dangling and the method Aig.cleanup_dangling so it’s clear how to
migrate.
---
Nitpick comments:
In `@python/aigverse/adapters/networkx.py`:
- Around line 175-179: Remove the redundant local binding src_int in the block
where you handle integer source nodes: instead of assigning src_int = src then
using src_int, construct the AigSignal directly from src (e.g., AigSignal(src,
bool(weight))) and use that signal for the subsequent self_named.has_name check;
update the references to use src and the newly created signal variable and
delete the unnecessary src_int declaration.
- Around line 91-92: The cast("NamedAig", self) after isinstance(self, NamedAig)
is redundant; either remove the cast and use the narrowed type directly (replace
self_named = cast("NamedAig", self) if isinstance(self, NamedAig) else None with
a simple conditional assignment using the narrowed self), or if this is a
pybind11 extension type that the type checker doesn't narrow, keep the cast but
add a one-line comment explaining the pybind11 narrowing workaround; update
occurrences around NamedAig, isinstance, and self_named in
python/aigverse/adapters/networkx.py accordingly.
In `@src/aigverse/algorithms/bindings.cpp`:
- Around line 3-11: The six bind_* forward declarations
(bind_equivalence_checking, bind_refactoring, bind_resubstitution,
bind_rewriting, bind_balancing, bind_simulation) live only in bindings.cpp which
risks signature drift; create a new header (e.g., algorithms_bindings.hpp) that
declares these functions taking pybind11::module_& and include that header both
in this bindings.cpp and in each corresponding definition file (e.g.,
bind_balancing.cpp) so mismatched signatures are caught at compile time; update
includes accordingly and remove the local forward declarations from
bindings.cpp.
In `@UPGRADING.md`:
- Line 14: The IO bullet in UPGRADING.md is vague and should enumerate the moved
function symbols; update the line about "Reading/Writing functions are now in
`aigverse.io`" to list the common functions that changed (for example
`read_aiger_into_aig`, `write_aiger`, and other frequently used
readers/writers), so users can grep and update imports; reference `aigverse.io`
as the new module and include the specific function names moved from their
previous locations.
Comment on lines
+148
to
+163
| # Iterate over synthetic PO nodes | ||
| for po_idx, _po in enumerate(self.pos()): | ||
| synth_node = po_idx + self.size() | ||
| attrs = {"index": synth_node} | ||
|
|
||
| # Synthetic PO attributes | ||
| type_vec = node_type_po | ||
| if levels: | ||
| attrs["level"] = depth_aig.num_levels() + 1 | ||
| if fanouts: | ||
| attrs["fanouts"] = 0 | ||
| if node_tts: | ||
| attrs["function"] = graph_funcs[po_idx] | ||
|
|
||
| attrs["type"] = type_vec | ||
| g.add_node(synth_node, **attrs) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "to_edge_list" --type=cpp -C 5Repository: marcelwa/aigverse
Length of output: 3794
🏁 Script executed:
rg -n "to_edge_list" --type=py -C 5Repository: marcelwa/aigverse
Length of output: 12604
🏁 Script executed:
fd -t f -e cpp -e h -e hpp | xargs grep -l "to_edge_list" | head -10Repository: marcelwa/aigverse
Length of output: 164
🏁 Script executed:
sed -n '127,200p' src/aigverse/adapters/edge_list.hppRepository: marcelwa/aigverse
Length of output: 2749
🏁 Script executed:
sed -n '165,191p' python/aigverse/adapters/networkx.pyRepository: marcelwa/aigverse
Length of output: 1358
Line 177: Remove redundant variable assignment
src_int = src is trivially redundant since src is already an integer from the edge tuple unpacking at line 169.
Also consider: On lines 185–191, when overwriting edge names with PO output names for synthetic PO nodes, the original signal name (set in the first loop, lines 174–180) is silently lost if both signal name and PO output name exist. Document this behavior or reconsider whether signal names should be preserved as a fallback when PO output names are unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/aigverse/adapters/networkx.py` around lines 148 - 163, Remove the
redundant src_int assignment and use src directly when creating edges (the
variables involved are src from the edge tuple and the synth_node computed as
po_idx + self.size()); additionally, when overwriting edge attributes for
synthetic PO nodes (the code that sets edge["name"] to PO output names for edges
targeting synth_node), preserve any existing signal name as a fallback—only
replace edge["name"] if the PO output name is non-empty/defined (or use a
setdefault-style behavior) so the original signal name set earlier is not
silently lost; update the logic in the loops that build edges and that
post-process PO-targeting edges (references: pos(), size(), po_idx, synth_node,
edge["name"], graph_funcs) accordingly.
Comment on lines
174
to
+191
| # Add signal name if available (edges represent signals) | ||
| if has_names: | ||
| from .. import AigSignal | ||
|
|
||
| # src is AigNode | int, convert to int for AigSignal constructor | ||
| src_int = src if isinstance(src, int) else self.node_to_index(src) | ||
| if self_named is not None: | ||
| # source node is an integer (AigNode) | ||
| src_int = src | ||
| sig = AigSignal(src_int, bool(weight)) | ||
| if self.has_name(sig): # type: ignore[attr-defined] | ||
| edge_attrs["name"] = self.get_name(sig) # type: ignore[attr-defined] | ||
| if self_named.has_name(sig): | ||
| edge_attrs["name"] = self_named.get_name(sig) | ||
|
|
||
| g.add_edge(src, tgt, **edge_attrs) | ||
|
|
||
| # Add PO names as attributes on edges going to synthetic PO nodes | ||
| if has_names and hasattr(self, "has_output_name") and hasattr(self, "get_output_name"): | ||
| for po_idx, po in enumerate(self.pos()): | ||
| if self.has_output_name(po_idx): | ||
| synthetic_po_node = self.po_index(po) + self.size() | ||
| if self_named is not None: | ||
| for po_idx, _ in enumerate(self_named.pos()): | ||
| if self_named.has_output_name(po_idx): | ||
| synthetic_po_node = po_idx + self_named.size() | ||
| # Find all edges going into this synthetic PO node | ||
| for pred in g.predecessors(synthetic_po_node): # type: ignore[no-untyped-call] | ||
| g.edges[pred, synthetic_po_node]["name"] = self.get_output_name(po_idx) | ||
| g.edges[pred, synthetic_po_node]["name"] = self_named.get_output_name(po_idx) |
There was a problem hiding this comment.
Signal name is silently overwritten by PO output name for edges entering synthetic PO nodes.
For an edge (src → synth_po_node) where both self_named.has_name(sig) and self_named.has_output_name(po_idx) are true:
- Lines 174–180 set
edge_attrs["name"] = self_named.get_name(sig)(signal/wire name). - Lines 185–191 unconditionally overwrite
g.edges[pred, synthetic_po_node]["name"] = self_named.get_output_name(po_idx)(PO output name).
The signal name is silently lost. If the intended semantic is "PO output name wins", add an explicit guard or a comment documenting the priority. If co-existence is desired, use distinct attribute keys (e.g., "signal_name" vs. "name").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/aigverse/adapters/networkx.py` around lines 174 - 191, The edge
attribute "name" assigned in the edge creation (around g.add_edge where
AigSignal(sig) and self_named.has_name(sig) are used) is later unconditionally
overwritten by the PO output assignment in the loop over self_named.pos();
change the attribute scheme to preserve both values: when setting the signal
name during edge creation, store it under a distinct key (e.g., "signal_name")
instead of "name" and when assigning PO output names to edges entering synthetic
PO nodes, store them under a distinct key (e.g., "output_name") (update the
g.add_edge block and the g.edges[pred, synthetic_po_node] assignment
accordingly) so the signal name is not silently lost; adjust any downstream
consumers if they expect the old "name" key.
| - **IO**: Reading/Writing functions are now in `aigverse.io`. | ||
| - **Utils**: `TruthTable`, `AigEdge`, `AigEdgeList`, `AigIndexList` and helper functions like `to_edge_list`, | ||
| `to_index_list`, `to_aig` are now in `aigverse.utils`. | ||
| - **Cleanup**: `cleanup_dangling` is exposed as the method `aigverse.networks.Aig.cleanup_dangling`. |
There was a problem hiding this comment.
cleanup_dangling promotion from free function to method needs a before/after calling convention example.
Describing it only as "exposed as the method aigverse.networks.Aig.cleanup_dangling" is ambiguous. Users migrating from cleanup_dangling(aig) (a free function call) won't immediately know the new syntax is aig.cleanup_dangling(). This is a more impactful breaking change than a simple re-path and deserves an explicit snippet.
📝 Suggested addition to the Key Changes section
-- **Cleanup**: `cleanup_dangling` is exposed as the method `aigverse.networks.Aig.cleanup_dangling`.
+- **Cleanup**: `cleanup_dangling` is now a method on `Aig` instead of a free function.
+ ```python
+ # Before
+ from aigverse import cleanup_dangling
+ cleanup_dangling(aig)
+
+ # After
+ aig.cleanup_dangling()
+ ```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Cleanup**: `cleanup_dangling` is exposed as the method `aigverse.networks.Aig.cleanup_dangling`. | |
| - **Cleanup**: `cleanup_dangling` is now a method on `Aig` instead of a free function. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@UPGRADING.md` at line 17, Add an explicit before/after calling example to the
UPGRADING.md entry for cleanup_dangling so users see the new instance method
syntax; replace or augment the current line mentioning
aigverse.networks.Aig.cleanup_dangling with a short Python fenced snippet
showing the old free-function call (from aigverse import cleanup_dangling;
cleanup_dangling(aig)) and the new method call (aig.cleanup_dangling()), and
ensure the example references the free function name cleanup_dangling and the
method Aig.cleanup_dangling so it’s clear how to migrate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request reorganizes the
aigversePython package into a modular structure, moving core classes and functions into dedicated submodules. It updates all documentation and code examples to use the new import paths, introduces a CMake helper for building Python bindings, and updates packaging configuration to reflect the new structure. An upgrade guide is also added to help users transition to the new API.API Refactoring and Documentation Updates:
All core classes and functions (e.g.,
Aig,balancing,write_aiger,TruthTable) are now organized into submodules:aigverse.networks,aigverse.algorithms,aigverse.io, andaigverse.utils. All documentation and code examples (inREADME.md,docs/*.md) have been updated to use these new import paths. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23] [24] [25] [26] [27] [28]An upgrade guide (
UPGRADING.md) is added, summarizing the new modular API and providing before/after code examples for migrating existing code.Build System and Packaging Changes:
A new CMake function (
add_aigverse_python_binding) is introduced to streamline building Python extension modules with pybind11.The Python package directory is moved from
src/aigversetopython/aigverse, and the build configuration inpyproject.tomlis updated to reflect the new module layout and targets. [1] [2] [3] [4]Resolves #124
Checklist:
Summary by CodeRabbit
New Features
Documentation
Breaking Changes
Tests
Chores