Conversation
denialhaag
added
python
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughLarge migration from pybind11 to nanobind: adds nanobind modules and registration functions, expands and tightens many Python .pyi stubs, renames several public Python symbols, updates build/CMake/packaging, and introduces stub generation and CI checks for stubs. Changes
Sequence Diagram(s)(Skipped — changes are broad binding/stub/build migrations without a single new multi-component sequential flow warranting a diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python/mqt/qmap/na/zoned.pyi (1)
129-143: Minor docstring style inconsistency between compiler classes.The
RoutingAgnosticCompilermethods use sentence case for argument descriptions (e.g., "The quantum circuit") whileRoutingAwareCompileruses the same style but with different capitalization patterns in the base docstrings. Since these stubs are auto-generated from C++ binding docstrings, this is a cosmetic issue that would need to be addressed in the source C++ bindings if consistency is desired.Based on learnings, stub files are auto-generated by nanobind's stubgen tool.
python/mqt/qmap/na/state_preparation.pyi (1)
151-171: MissingReturns:section ingenerate_codedocstring.The
generate_codefunction docstring documentsArgsandRaisesbut is missing aReturns:section to describe the return value (str). Since stubs are auto-generated, this should be fixed in the C++ binding docstring inbindings/na/nasp/nasp.cpp.bindings/na/zoned/zoned.cpp (1)
173-193: Update docstrings to reflect actual exception behavior or register custom exception translator.The docstrings document that
ValueErroris raised for invalid JSON, butnlohmann::json::parse()throwsnlohmann::json::parse_error, which derives fromnlohmann::json::exception—not a standard C++ exception. By default, nanobind translates unregistered exceptions toRuntimeError, notValueError. Users expectingValueErrorwill not catch the actual exception.Fix by either:
- Updating the docstrings to document
RuntimeErrorinstead ofValueError, or- Registering a custom nanobind exception translator to convert
nlohmann::json::parse_errortoValueErrorThis issue affects:
Architecture::from_json_file(line 53)Architecture::from_json_string(line 66)RoutingAgnosticCompiler::from_json_string(line 192–193)RoutingAwareCompiler::from_json_string(line 344–345)
🤖 Fix all issues with AI agents
In `@bindings/na/nasp/nasp.cpp`:
- Around line 119-129: The docstring for the .def("json", ...) wrapper around
na::NASolver::Result::json is inaccurate: the implementation uses Python's
json.loads and returns a JSON-compatible Python object (dict/list), not a JSON
string. Update the docstring for the .def("json", ...) method to state it
returns a JSON-compatible Python object (e.g., "Returns the result as a
JSON-compatible Python object") and adjust the Returns: line accordingly so it
matches the actual behavior of the lambda that calls
json.loads(result.json().dump()).
- Around line 141-154: Update the generate_code docstring to include a Returns:
section describing the return value; specifically, in the docstring for function
generate_code (the block that currently lists Args including qc, result,
min_atom_dist, no_interaction_radius, zone_dist) add a Returns: entry stating
that the function returns a str containing the generated code for the given
circuit (or similar concise description), and optionally note the format or
content of the string if needed.
In `@bindings/na/zoned/zoned.cpp`:
- Around line 201-208: Docstring typo: change "compilations result" to
"compilation result" inside the pybind raw string literals used for the binding
that takes "qc"_a (the R"pb(... .naviz format.)pb" docstring) and the second
identical raw-string docstring later (the other R"pb(... )pb" around line 360)
so both occurrences read "The compilation result as a string in the .naviz
format." Ensure you update both raw string literals associated with the binding
that uses "qc"_a.
- Around line 159-170: Update the docstring for the routing-agnostic compiler
binding to clarify accepted log-level formats: state that spdlog accepts both
long names ("debug", "info", "warning", "error", "critical") and short
single-letter forms ("D", "I", "W", "E", "C") and note the default is "I"
(info); apply the same clarification to the RoutingAwareCompiler binding
docstring. Reference the binding functions/docstrings for
create_routing_agnostic_compiler (the routing-agnostic compiler docstring shown)
and RoutingAwareCompiler to locate and edit the text accordingly.
In `@bindings/sc/sc.cpp`:
- Line 228: The binding change renamed the Python attribute to "input_" causing
a breaking API change; restore backward compatibility by exposing the original
name as an alias on the MappingResults pybind class (in addition to the new
"input_") so existing code can still access MappingResults.input; update the
binding where def_rw("input_", &MappingResults::input) is defined to also define
"input" (or a def_property/deprecated shim that delegates to
MappingResults::input and emits a deprecation warning) so both names map to the
same C++ member.
In `@python/mqt/qmap/clifford_synthesis.pyi`:
- Around line 209-211: Fix the docstring typo in the overloaded constructor for
__init__ (the signature overload in clifford_synthesis.pyi taking stabilizers:
str, destabilizers: str) by updating the C++ binding source so the docstring
reads "Stabilizers and Destabilizers" (i.e., insert the missing space between
"Stabilizers" and "and") so the generated Python .pyi shows the corrected text
for that constructor overload.
- Around line 192-196: Update the docstrings for the methods sat() and unsat()
in clifford_synthesis.pyi to use Python boolean literal casing (True/False)
instead of lowercase "true"/"false": locate the def sat(self) -> bool: and def
unsat(self) -> bool: stubs and change their docstrings to say "Returns `True` if
the synthesis was successful." and "Returns `False` if the synthesis was
unsuccessful." respectively.
- Around line 69-74: The docstring for the linear_search property in the Python
stub contains a typo ("liner" → "linear"); update the source of truth in the C++
binding generation so the generated docstring for the linear_search property
(getter/setter) reads "Use linear search instead of binary search scheme for
finding the optimum. Defaults to `false`." and regenerate the stub so both the
`@property` def linear_search and its setter carry the corrected text.
In `@python/mqt/qmap/na/state_preparation.pyi`:
- Around line 69-78: The stub for Result.json has an incorrect, overly specific
return annotation (dict[str, Any])—it should be a generic object to match the
C++ binding (nb::object returned from json.loads); regenerate the
nanobind-generated stubs (run the project's stub generation script) or update
the stub so Result.json() on the NA state preparation stub uses return type
object to match the other state_preparation stub and the C++ binding (refer to
Result.json and the C++ nasp binding that returns nb::object).
In `@python/mqt/qmap/state_preparation.pyi`:
- Around line 124-144: The generate_code function's docstring lacks a Returns
section describing its str return value; update the doc comments for
generate_code (the binding that populates python/mqt/qmap/state_preparation.pyi)
to include a "Returns:" entry stating it returns the generated code as a str
(and any format/encoding expectations), and mirror that change in the
corresponding C++ binding/source doc comment so the pyi is regenerated with the
new Returns description.
- Around line 32-65: The stub's docstring for the neutral-atom state preparation
solver (the docstring that documents parameters max_x, max_y, max_c, max_r,
max_h_offset, max_v_offset, max_h_dist, max_v_dist, min_entangling_y,
max_entangling_y) has inconsistent indentation between the main body (8 spaces)
and the Note/Args/Raises sections (4 spaces); fix the C++ binding source raw
string in the nasp binding (the raw docstring in bindings/na/nasp/nasp.cpp) so
the literal uses consistent indentation (prefer 4-space indentation for all
sections) or apply a dedent/format step when emitting the Python docstring,
ensuring the updated docstring regenerates the .pyi with uniform indentation.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@bindings/na/state_preparation.cpp`:
- Around line 161-170: The lowercase transformation result opTypeLowerStr is
computed but never used; update the lambda in get_ops_for_solver to pass the
lowered string to qc::opTypeFromString (i.e., call
qc::opTypeFromString(opTypeLowerStr)) so case-insensitive matching works, or
alternatively remove the transformation if you intend to keep case-sensitive
behavior; ensure NASolver::getOpsForSolver still receives the correct op type
derived from the lowered string.
In `@bindings/na/zoned.cpp`:
- Around line 369-372: There is an extraneous semicolon immediately after the
return statement inside the lambda that returns nb::cast<nb::typed<nb::dict,
nb::str, nb::float_>>(dict); remove the trailing semicolon so the lambda ends
cleanly (i.e., delete the standalone ";" following the return); verify the
lambda compiles and retains the nb::cast usage as-is.
In `@python/mqt/qmap/clifford_synthesis.pyi`:
- Around line 63-140: Update the Python-facing docstrings for the listed
properties to use Python boolean casing "False" instead of "false"; specifically
edit the docstrings for properties use_maxsat, linear_search,
dump_intermediate_results, minimize_gates_after_depth_optimization,
try_higher_gate_limit_for_two_qubit_gate_optimization,
minimize_gates_after_two_qubit_gate_optimization, and heuristic so their
"Defaults to `false`" becomes "Defaults to `False`". Make this change in the C++
binding source that generates these stubs (the code that produces the docstrings
for the properties named above) so the generated .pyi reflects the Python
boolean convention and matches use_symmetry_breaking's "True".
- Around line 239-240: Add a missing docstring for the result_circuit property
on the Cliffordsynthesis binding: update the property definition that exposes
result_circuit (the getter for result_circuit) to include a descriptive
docstring such as "Returns the synthesized circuit as a QuantumComputation
object." so the Python stub and runtime binding both have consistent
documentation for the result_circuit property.
- Around line 63-68: Fix the typo in the docstring for the use_maxsat property:
change "really on the binary search scheme" to "rely on the binary search
scheme". Update the documentation string in the C++ nanobind binding that
defines the use_maxsat property/getter/setter (the binding for use_maxsat in the
clifford_synthesis binding source) so the generated stub and pyi will include
the corrected phrase; do not edit the autogenerated .pyi directly.
♻️ Duplicate comments (1)
bindings/sc/sc.cpp (1)
229-229: Breaking API change already flagged.The rename from
inputtoinput_was flagged in a previous review. This is a breaking change that will require users to update their code fromMappingResults.inputtoMappingResults.input_.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/clifford_synthesis/clifford_synthesis.cpp (1)
216-218: Add docstring forresult_circuitproperty.The
result_circuitproperty lacks a docstring, unlike other properties in this class. This will result in the auto-generated stub missing documentation for this property.🔧 Proposed fix
synthesizer.def_prop_ro("result_circuit", [](cs::CliffordSynthesizer& self) { return qasm3::Importer::imports(self.getResults().getResultCircuit()); - }); + }, + "Returns the synthesized circuit as a QuantumComputation object.");
🤖 Fix all issues with AI agents
In `@python/mqt/qmap/clifford_synthesis.pyi`:
- Around line 213-240: The stub lacks a docstring for the result_circuit
property because the auto-generated pyi comes from the C++ binding; update the
binding in bindings/clifford_synthesis/clifford_synthesis.cpp by supplying a
docstring for the result_circuit exposure on the pybind11 class for
CliffordSynthesizer (the def_property_readonly / def_property that exposes
result_circuit) so the generated Python stub includes the same descriptive
docstring pattern used for synthesize and results; ensure the string describes
that result_circuit returns the synthesized mqt.core.ir.QuantumComputation
result.
♻️ Duplicate comments (1)
bindings/na/zoned.cpp (1)
159-170: Consider clarifying log level short forms in the docstring.The docstring mentions
"debug", "info", "warning", "error", "critical"as possible values, but the default is"I"(the spdlog short form for "info"). Whilespdlog::level::from_str()accepts both forms, users may be confused by the mismatch.🔧 Proposed fix (applies to both compilers)
- log_level: The log level for the compiler, possible values are "debug", "info", "warning", "error", "critical" + log_level: The log level for the compiler, possible values are "debug"/"D", "info"/"I", "warning"/"W", "error"/"E", "critical"/"C"
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bindings/na/register_zoned.cpp`:
- Around line 210-218: The current "stats" binding casts the returned Python
dict to nb::typed<nb::dict, nb::str, nb::float_>, which forces all values to
float and can lose ints/bools/nested structures; change the cast to use a
generic container (nb::any or nb::object) so types are preserved: in the lambda
passed to routingAgnosticCompiler.def("stats", ...) replace nb::typed<nb::dict,
nb::str, nb::float_> with nb::object (or nb::any) and do the same for the
analogous binding around lines 363-371, keeping use of self.getStatistics() and
json.loads(stats.dump()) but returning the generic nb type instead of float-only
typed dict.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Nice. This looks great to me! I'll leave it to you whether you actually want to perform the breaking renaming changes.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/mqt/qmap/plugins/qiskit/sc/compile.py (1)
44-69: Makeinclude_wcnfand following parameters keyword-only to avoid FBT warnings and prevent accidental positional misuse in the long function signature.Add
*separator beforeinclude_wcnf(line 63). All existing call sites across the codebase and documentation examples already use keyword arguments for trailing parameters, so this change is safe and has no breaking impact.♻️ Proposed change
def compile_( circ: CircuitInputType, arch: str | Arch | Architecture | BackendV2 | None, @@ commander_grouping: CommanderGrouping = CommanderGrouping.fixed3, swap_reduction: SwapReduction = SwapReduction.coupling_limit, swap_limit: int = 0, + *, include_wcnf: bool = False, use_subsets: bool = True, subgraph: set[int] | None = None,docs/mapping.md (1)
152-152: Update the function reference for consistency.The note references
`compile`method but the function has been renamed tocompile_. This should be updated to maintain consistency with the API change.Proposed fix
-Such an optimization pass is conducted by default in the `compile` method after the circuit has been mapped. +Such an optimization pass is conducted by default in the `compile_` method after the circuit has been mapped.
♻️ Duplicate comments (1)
bindings/sc/sc.cpp (1)
226-231: Keep a backward-compatibleinputalias for MappingResults.Renaming to
input_alone breaks existing code; retain an alias (optionally deprecated).🔧 Suggested compatibility shim
nb::class_<MappingResults>(m, "MappingResults", "Class representing the results of a mapping.") .def(nb::init<>()) .def_rw("input_", &MappingResults::input) + .def_rw("input", &MappingResults::input) // TODO: deprecate later .def_rw("output", &MappingResults::output)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/mapping.md (1)
174-174: Fix phrasing: “orders of magnitude.”Use the standard phrase “orders of magnitude” (singular “magnitude”).
2 participants
Description
After we have switched from
pybind11tonanobindin #911, we can now auto-generate the stub files. This PR defines a correspondingnoxsession and copies over all existing docstrings to the bindings code.Checklist:
I have added appropriate tests that cover the new/changed functionality.I have updated the documentation to reflect these changes.