🐛 Value of qubits of state variables are now editable in syrec-editor and correctly matched in actual/expected output state matching#540
Open
TooMuchDakka wants to merge 23 commits intomunich-quantum-toolkit:mainfrom
Conversation
… as ancillary quantum registers. Start of refactoring of annotatable quantum computation tests
…tion now also check the contents of the inline qubit information instead of only checking whether said inline information can be fetched
…um circuit simulation dialog due to qubits for local variables of SyReC module now being correctly marked as ancillary ones
…e synthesized SyReC programs
…ting to create quantum registers storing data qubits. Attempting to fetch the user declared qubit label in a quantum register storing data qubits is no longer automatically discarded simply based on the stored qubit type in the qreg
… that should be added via separate PR
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…as local module variables of type 'wire' are now also marked as garbage
|
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:
📝 WalkthroughWalkthroughIntroduces explicit QubitType (Data, Garbage, Ancillary); refactors quantum-register/layout types and APIs to carry qubit-type information; updates synthesis to map SyReC variable kinds to QubitType; propagates data-qubit lookup through Python simulation UI, workers, models, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Synth as Synth (syrec_synthesis.cpp)
participant Var as SyReC_Variable
participant Mapper as getQubitTypeAssociatedWithSyrecVariableType
participant AQC as AnnotatableQuantumComputation
participant Worker as Python Simulation Workers
Synth->>Var: inspect variable.type
Var->>Mapper: request mapping
Mapper-->>Synth: QubitType (Data/Garbage/Ancillary) or nullopt
alt mapping OK
Synth->>AQC: addQuantumRegisterForSyrecVariable(QubitType, label, layoutInfo, inlinedInfo, forceInline)
AQC->>AQC: create QuantumRegisterForVariableLayout (type stored)
Synth->>AQC: addPreliminaryAncillaryRegisterAggregatingIntermediateResultsOrAppendToAdjacentOne(...)
AQC-->>Synth: preliminary ancillary ranges recorded
Synth->>AQC: promoteQuantumRegistersPreliminaryMarkedAsStoringAncillaryQubitsToDefinitivelyStoringAncillaryQubits()
AQC->>AQC: filter registers by QubitType::Ancillary and mark definitive
AQC->>Worker: expose DataQubitsLookup for simulation flows
else mapping failed
Synth-->>Synth: abort synthesis
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/algorithms/synthesis/syrec_synthesis.cpp (2)
1523-1531:⚠️ Potential issue | 🟠 MajorGuard optional before dereference in
getConstantLines.If
addPreliminaryAncillaryRegisterAggregatingIntermediateResultsOrAppendToAdjacentOne(...)returnsstd::nullopt, the code still dereferences the optional to build ranges, which is UB. Early-return when the optional is empty or mismatched.🛠️ Suggested fix
- const std::optional<qc::Qubit> actualQubitIndexForFirstAddedAncillaryQubit = annotatableQuantumComputation.addPreliminaryAncillaryRegisterAggregatingIntermediateResultsOrAppendToAdjacentOne(quantumRegisterLabel, initialValuesOfAncillaryQubits, inliningInformation); - const bool couldAncillaryQubitsBeAdded = actualQubitIndexForFirstAddedAncillaryQubit.has_value() && *actualQubitIndexForFirstAddedAncillaryQubit == expectedQubitIndexForFirstAddedAncillaryQubit; - - const qc::Qubit firstGeneratedAncillaryQubitIndex = *actualQubitIndexForFirstAddedAncillaryQubit; + const std::optional<qc::Qubit> actualQubitIndexForFirstAddedAncillaryQubit = + annotatableQuantumComputation.addPreliminaryAncillaryRegisterAggregatingIntermediateResultsOrAppendToAdjacentOne( + quantumRegisterLabel, initialValuesOfAncillaryQubits, inliningInformation); + if (!actualQubitIndexForFirstAddedAncillaryQubit.has_value() || + *actualQubitIndexForFirstAddedAncillaryQubit != expectedQubitIndexForFirstAddedAncillaryQubit) { + return false; + } + + const qc::Qubit firstGeneratedAncillaryQubitIndex = *actualQubitIndexForFirstAddedAncillaryQubit; ... - return couldAncillaryQubitsBeAdded; + return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/algorithms/synthesis/syrec_synthesis.cpp` around lines 1523 - 1531, The code dereferences actualQubitIndexForFirstAddedAncillaryQubit without checking it may be std::nullopt; in getConstantLines (or the function containing this snippet) add an early return when actualQubitIndexForFirstAddedAncillaryQubit is empty or when couldAncillaryQubitsBeAdded is false (i.e., if !actualQubitIndexForFirstAddedAncillaryQubit || !couldAncillaryQubitsBeAdded) before computing firstGeneratedAncillaryQubitIndex/lastGeneratedAncillaryQubitIndex and the loop that emplaces into lines so you never dereference a null optional returned from addPreliminaryAncillaryRegisterAggregatingIntermediateResultsOrAppendToAdjacentOne.
1441-1453:⚠️ Potential issue | 🟠 MajorState variables still get an ancillary-style internal label.
This block assigns an internal label via
InternalQubitLabelBuilder::buildNonAncillaryQubitLabel(...)for bothWireandState. WithStatenow mapped toQubitType::Data, using the__q-style internal prefix risks downstream classification as ancillary (e.g., UI/label-based checks). Consider using a non-ancillary prefix forStatevariables or updating downstream logic to rely onQubitTypeinstead of the label prefix.
Based on learnings: In the munich-quantum-toolkit/syrec repository, the__qprefix in internal qubit labels indicates ancillary qubits, and_does_qubit_label_start_with_internal_qubit_label_prefixreturns true for ancillary qubits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/algorithms/synthesis/syrec_synthesis.cpp` around lines 1441 - 1453, The code assigns the same internal "__q" style label to both Wire and State (in the block around variable->type check using InternalQubitLabelBuilder::buildNonAncillaryQubitLabel), which causes State (now mapped to QubitType::Data) to be misclassified as ancillary; fix by branching on Variable::Type::State and generating a true non-ancillary/internal data label instead of the "__q" ancillary-style prefix (either call or add an InternalQubitLabelBuilder method tailored for Data/State qubits or use a different prefix), and ensure AnnotatableQuantumComputation::InlinedQubitInformation and any label-based checks rely on QubitType::Data rather than the label prefix where possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.clang-tidy:
- Around line 7-10: The rule clang-analyzer-optin.core.EnumCastOutOfRange has
been disabled (prefixed with a '-') in the .clang-tidy config; if that was
accidental, re-enable it by removing the leading '-' from
clang-analyzer-optin.core.EnumCastOutOfRange so the analyzer checks enum cast
ranges again, or if the intent is only to avoid auto-correcting typos keep the
check enabled and instead add specific identifiers to your typos exclusion list;
locate and update the clang-analyzer-optin.core.EnumCastOutOfRange entry to
reflect the chosen approach.
In `@include/core/annotatable_quantum_computation.hpp`:
- Around line 175-195: The docs for addQuantumRegisterForSyrecVariable and the
ancillary-register API are stale: update the `@return` text to reflect the current
return semantics (e.g., state that the function returns the index of the first
generated qubit as std::optional<qc::Qubit> and under which failure conditions
it returns std::nullopt) and remove the phrase “non-ancillary qubit”; also
replace the obsolete reference to the old promotion method name in the remarks
with the current API name used to promote preliminary ancillary qubits (ensure
the remark describes promotion via the current AnnotatableQuantumComputation
promotion method), and adjust any wording that assumed old behavior so the
parameter descriptions, `@return` and `@remark` match the renamed APIs and actual
semantics.
In `@test/unittests/test_generated_quantum_computation_by_synthesis.cpp`:
- Line 79: The assertion's failure message prints the wrong expected variable;
change the message to reflect the actual expected expression used in the
comparison (shouldQubitsBeMarkedAsAncillary | shouldQubitsBeMarkedAsGarbage) so
failures correctly report the expected garbage classification; update the
ASSERT_EQ message referencing
annotatableQuantumComputation.logicalQubitIsGarbage(qubit) to print the combined
expected value (or compute an intermediate expectedIsGarbage variable from
shouldQubitsBeMarkedAsAncillary and shouldQubitsBeMarkedAsGarbage and use that
in both the ASSERT_EQ left operand and the diagnostic string).
- Line 50: Change the parameter type of
parseInputCircuitPerformSynthesisAndAssertSuccess from "const std::string_view&
stringifiedSyrecProgram" to pass the view by value: "std::string_view
stringifiedSyrecProgram"; update any call sites to pass the same argument (no
other changes needed since string_view is non-owning) so the function takes the
lightweight view directly instead of an unnecessary const reference.
- Around line 74-82: In assertQubitsInRangeAreCorrectlyMarkedAsQubitsOfType,
replace the bitwise OR usage (shouldQubitsBeMarkedAsAncillary |
shouldQubitsBeMarkedAsGarbage) with the idiomatic boolean logical OR
(shouldQubitsBeMarkedAsAncillary || shouldQubitsBeMarkedAsGarbage) and add a
brief inline comment explaining the domain rule that ancillary qubits are also
considered garbage (i.e., isGarbage == isAncillary || isGarbage) so future
readers understand why the garbage check combines both flags; refer to the
function name assertQubitsInRangeAreCorrectlyMarkedAsQubitsOfType and the
methods logicalQubitIsGarbage/logicalQubitIsAncillary to locate the assertions.
---
Outside diff comments:
In `@src/algorithms/synthesis/syrec_synthesis.cpp`:
- Around line 1523-1531: The code dereferences
actualQubitIndexForFirstAddedAncillaryQubit without checking it may be
std::nullopt; in getConstantLines (or the function containing this snippet) add
an early return when actualQubitIndexForFirstAddedAncillaryQubit is empty or
when couldAncillaryQubitsBeAdded is false (i.e., if
!actualQubitIndexForFirstAddedAncillaryQubit || !couldAncillaryQubitsBeAdded)
before computing
firstGeneratedAncillaryQubitIndex/lastGeneratedAncillaryQubitIndex and the loop
that emplaces into lines so you never dereference a null optional returned from
addPreliminaryAncillaryRegisterAggregatingIntermediateResultsOrAppendToAdjacentOne.
- Around line 1441-1453: The code assigns the same internal "__q" style label to
both Wire and State (in the block around variable->type check using
InternalQubitLabelBuilder::buildNonAncillaryQubitLabel), which causes State (now
mapped to QubitType::Data) to be misclassified as ancillary; fix by branching on
Variable::Type::State and generating a true non-ancillary/internal data label
instead of the "__q" ancillary-style prefix (either call or add an
InternalQubitLabelBuilder method tailored for Data/State qubits or use a
different prefix), and ensure
AnnotatableQuantumComputation::InlinedQubitInformation and any label-based
checks rely on QubitType::Data rather than the label prefix where possible.
test/unittests/test_generated_quantum_computation_by_synthesis.cpp
Outdated
Show resolved
Hide resolved
test/unittests/test_generated_quantum_computation_by_synthesis.cpp
Outdated
Show resolved
Hide resolved
…QubitBuilds non-ancillary label. Fixed std::optional dereference without checking whether the std::optional has a value bug
…o correctly consider data qubits for SyReC variables of type 'state'
… execution controls in CRUD operations as well as tab switches
…rs potential gap between 'data qubits' generated by qubits of local SyReC module variable of type 'state'
TooMuchDakka
changed the title
wire are now generated as ancillary onesstate variables are now editable in syrec-editor and correctly matched in actual/expected output state matching
TooMuchDakka
added
python
Collaborator
Author
|
@burgholzer I have fixed the issues reported by the CodeRabbit ai outside of the diff range and tried to re-request a review but the action still requests changes. I could dismiss the review but would have only done that after your review OK. |
Member
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
burgholzer
requested changes
Feb 19, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Haven't checked every single detail here, but this generally looks good.
Just two smaller comments before this can go in.
| */ | ||
| Garbage, | ||
| /** | ||
| * Qubit is used to store intermediate results of the quantum computation and its final state is not relevant for the computation performed by the quantum computation. |
Member
There was a problem hiding this comment.
That's not really the definition of an ancillary qubit. The only definition is that the qubit is guaranteed to start in |0>. In particular, ancillary qubits may hold output values (e.g., if swapped from a data qubit due to physical constraints).
Collaborator
Author
There was a problem hiding this comment.
I have update the comment for the QubitType::Ancillary but just wanted to double-check that the "classification" of the generated ancillary qubits as both ancillary/garbage is still correct? A swap of an ancillary qubit with a data qubit should also transfer this "classification" between the two qubits?
Member
There was a problem hiding this comment.
The classification of whether a qubit is ancillary/garbage or not is a static property of the circuit in general; it does not dynamically change over the course of a circuit.
qubit q;
qubit anc;
h q;
swap q, anc;
bit c = measure anc;
While a bit constructed; in the above circuit, anc is an ancillary qubit, but not a garbage qubit (it is measured in the end). Similarly, q is a data qubit, but also a garbage qubit.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp (1)
88-105:⚠️ Potential issue | 🟠 MajorString-to-qubit index mapping breaks with non-ancillary gaps.
When non-ancillary qubits are non-contiguous (e.g., {0, 2, 4}),
loadNBitValuesContainerFromString()maps the input string to sequential indices [0..len) instead of the actual non-ancillary qubit indices. At line 104,assertSimulationResultForStateMatchesExpectedOne()then accesses the container using the actual qubit indices, causing a mismatch. For example, if the string is "101" representing qubits {0, 2, 4}, the code stores bits at [0, 1, 2] but later checks [0, 2, 4], comparing the wrong values.The assertion at line 97 checks only that the string length does not exceed total qubits, not that it matches the non-ancillary qubit count. Either require full-length strings with validation that
max(nonAncillaryQubitsLookup) < inputState.size(), or map short strings to non-ancillary qubit indices in ascending order.Also applies to: 177-188, 204-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp` around lines 88 - 105, The input string bits are being loaded into sequential indices by loadNBitValuesContainerFromString but later compared against non-contiguous qubit indices from generateNonAncillaryQubitsLookup in assertSimulationResultForStateMatchesExpectedOne; fix by changing the loading logic (or its call site) to map the characters of jsonKeyForStringifiedBinaryInputState (and output) to the sorted nonAncillaryQubitsLookup indices in ascending order (i.e., assign bit 0 -> smallest non-ancillary qubit, bit 1 -> next, etc.), and add a guard that the input string length equals nonAncillaryQubitsLookup.size() (or reject/normalize otherwise) so comparisons in assertSimulationResultForStateMatchesExpectedOne use the correct qubit positions.python/mqt/syrec/simulation_view/dialogs/all_input_states_generator_dialog.py (1)
66-79:⚠️ Potential issue | 🟡 MinorMisleading type annotation and incorrect comment:
2 ** intreturnsintin Python, notfloat.The annotation
Final[float]forn_expected_sim_runs_to_generateis semantically wrong — Python's**operator on twointoperands returns anint(arbitrary precision). The preceding comment ("operands of the exponentiation operator are casted to floats") is factually incorrect for Python and should be removed. Theint(...)casts on lines 72 and 78 are therefore also redundant.🛠️ Proposed fix
- # Since the operands of the exponentiation operator are casted to floats, the result (a float) could exceed the maximum storable value in an integer so we need to check this error case since - # otherwise setting the maximum value of the QtWidgets.QProgressBar would raise an exception/no matching overload will be found since said function expects an integer parameter. - n_expected_sim_runs_to_generate: Final[float] = 2 ** len( + # 2 ** n can grow large; guard against exceeding QProgressBar's 32-bit max before calling setMaximum(). + n_expected_sim_runs_to_generate: Final[int] = 2 ** len( self._data_qubits_lookup.ascendingly_ordered_data_qubits_lookup ) if self._progress_bar is not None: - if not self._can_value_can_be_used_as_progress_bar_max_value(int(n_expected_sim_runs_to_generate)): + if not self._can_value_can_be_used_as_progress_bar_max_value(n_expected_sim_runs_to_generate): # We do not ask for confirmation to close the dialog since we faulted before the input state generation started. super().reject() return self._progress_bar.setMinimum(0) - self._progress_bar.setMaximum(int(n_expected_sim_runs_to_generate)) + self._progress_bar.setMaximum(n_expected_sim_runs_to_generate)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/mqt/syrec/simulation_view/dialogs/all_input_states_generator_dialog.py` around lines 66 - 79, The variable n_expected_sim_runs_to_generate is declared/used as a float but 2 ** len(...) yields an int in Python; change its annotation to Final[int], remove the incorrect comment about float casting, and stop wrapping it in int(...) when calling _can_value_can_be_used_as_progress_bar_max_value and _progress_bar.setMaximum — use n_expected_sim_runs_to_generate directly (or ensure an explicit int() only if the callee signature requires it). Keep the existing early reject via super().reject() and the progress bar min/value calls intact.
🤖 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/mqt/syrec/quantum_circuit_simulation_dialog.py`:
- Around line 1260-1267: The parameter type for curr_active_tab_widget in
_set_enabled_state_of_simulation_run_execution_control_btn_in_tab_based_on_sim_run_execution_mode
is incorrect: replace QtWidgets.QTabWidget with QtWidgets.QWidget (since callers
pass tab content widgets like the QFrame from
initialize_simulation_runs_tab_widget, not the QTabWidget container) and update
any related type hints or docstrings referencing that parameter to avoid
mypy/pyright errors.
In `@python/mqt/syrec/simulation_view/simulation_run_model.py`:
- Around line 396-400: The helper _are_data_qubits_in_sequence_without_gaps
currently compares an element to itself plus one (tautology) causing it to
always return True; change the comparison to compare the current element to the
previous element (sorted_data_qubits_indices_collection[idx] !=
sorted_data_qubits_indices_collection[idx - 1] + 1) so it correctly detects
gaps, and fix the function's boolean semantics/name to match its use: either
invert the return value or rename the function to
_do_gaps_exist_between_data_qubits and update the single call site in
_build_data_qubits_lookup (where do_gaps_between_data_qubits is set) so the
stored flag reflects gaps correctly.
- Around line 40-43: The DataQubitsLookup dataclass is declared frozen but uses
a mutable list which breaks immutability and hashability; change the field type
from list[int] to tuple[int, ...] in the DataQubitsLookup definition and ensure
the factory in _build_data_qubits_lookup constructs and assigns a tuple (e.g.
tuple(ascendingly_ordered_data_qubits_lookup)) instead of a list so instances
are truly immutable and hashable.
In
`@python/mqt/syrec/simulation_view/workers/all_input_states_generator_worker.py`:
- Around line 119-125:
QtSimulationRunModel._are_data_qubits_in_sequence_without_gaps currently returns
the wrong value (setting do_gaps_between_data_qubits_exist to True
unconditionally), which prevents the fast-path in
all_input_states_generator_worker from being used; fix
_are_data_qubits_in_sequence_without_gaps so it inspects
QtSimulationRunModel.ascendingly_ordered_data_qubits_lookup (or the same ordered
list used elsewhere), verifies each successive data qubit index equals the
previous index + 1 (no gaps), and return True only when the sequence is
contiguous (False if any gap); ensure do_gaps_between_data_qubits_exist is
derived from this method (or its negation) consistently so the fast-path in
all_input_states_generator_worker can be taken when there are no gaps.
---
Outside diff comments:
In
`@python/mqt/syrec/simulation_view/dialogs/all_input_states_generator_dialog.py`:
- Around line 66-79: The variable n_expected_sim_runs_to_generate is
declared/used as a float but 2 ** len(...) yields an int in Python; change its
annotation to Final[int], remove the incorrect comment about float casting, and
stop wrapping it in int(...) when calling
_can_value_can_be_used_as_progress_bar_max_value and _progress_bar.setMaximum —
use n_expected_sim_runs_to_generate directly (or ensure an explicit int() only
if the callee signature requires it). Keep the existing early reject via
super().reject() and the progress bar min/value calls intact.
In `@test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp`:
- Around line 88-105: The input string bits are being loaded into sequential
indices by loadNBitValuesContainerFromString but later compared against
non-contiguous qubit indices from generateNonAncillaryQubitsLookup in
assertSimulationResultForStateMatchesExpectedOne; fix by changing the loading
logic (or its call site) to map the characters of
jsonKeyForStringifiedBinaryInputState (and output) to the sorted
nonAncillaryQubitsLookup indices in ascending order (i.e., assign bit 0 ->
smallest non-ancillary qubit, bit 1 -> next, etc.), and add a guard that the
input string length equals nonAncillaryQubitsLookup.size() (or reject/normalize
otherwise) so comparisons in assertSimulationResultForStateMatchesExpectedOne
use the correct qubit positions.
python/mqt/syrec/simulation_view/workers/all_input_states_generator_worker.py
Show resolved
Hide resolved
…xist, reworded comment regarding check that number of to be generated input states does not exceed maximum value of QProgressBar. Made DataQubitsLookup dataclass readonly
…ect.toml instead of excluding .clang-tidy file in .pre-commit-config.yaml
… from the to be checked input state
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/mqt/syrec/quantum_circuit_simulation_dialog.py (1)
1147-1148:⚠️ Potential issue | 🟡 MinorType annotations should be
QtWidgets.QWidget, notQTabWidget.Both methods are annotated with
QTabWidgetbut all call sites passQWidget-typed arguments:
- Line 783 calls
set_default_simulation_run_modification_buttons_enabled_state(prev_active_tab_widget)whereprev_active_tab_widget: Final[QtWidgets.QWidget]- Line 767 and 982 call
_clear_simulation_run_list_and_backing_model()withQWidget-typed argumentsAdditionally, the operations performed on these parameters (
objectName(),findChild()) areQWidgetmethods, notQTabWidget-specific.🛠️ Proposed fix
def set_default_simulation_run_modification_buttons_enabled_state( - self, associated_tab_widget: QtWidgets.QTabWidget + self, associated_tab_widget: QtWidgets.QWidget ) -> None:def _clear_simulation_run_list_and_backing_model( - self, tab_widget_containing_list_view: QtWidgets.QTabWidget + self, tab_widget_containing_list_view: QtWidgets.QWidget ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/mqt/syrec/quantum_circuit_simulation_dialog.py` around lines 1147 - 1148, The type annotations for the methods set_default_simulation_run_modification_buttons_enabled_state and _clear_simulation_run_list_and_backing_model should use QtWidgets.QWidget (not QtWidgets.QTabWidget) because all call sites pass QWidget instances and the code only uses QWidget methods like objectName() and findChild(); update the function signatures and any associated parameter annotations to QtWidgets.QWidget so the types match callers and the used methods are valid for the annotated type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.clang-tidy:
- Line 9: Replace the misspelled check name
"clang-analyzer-option.core.EnumCastOutOfRange" in the .clang-tidy file with the
correct "clang-analyzer-optin.core.EnumCastOutOfRange" so the disable directive
actually applies; locate the line containing the string
"clang-analyzer-option.core.EnumCastOutOfRange" and update it to use "optin"
(matching the clang-tidy check name) so the optin check is properly disabled.
In `@include/core/annotatable_quantum_computation.hpp`:
- Line 197: The method name
addPreliminaryAncillaryRegisterAggregatingIntermediateResultsOrAppendToAdjacentOne
is excessively long; rename it to a shorter, still-descriptive identifier (for
example addPrelimAncillaOrAppendAdjacent) and update its declaration in
AnnotatableQuantumComputation (the function signature shown) along with every
implementation and all call sites, comments and tests that reference the old
name; ensure you keep the same signature/semantics and add a deprecated
forwarding wrapper with the old name if you need a transitional period for
callers.
- Around line 415-418: Update the stale documentation on
QuantumRegisterForVariableLayout: change the class comment that currently says
"assumed to not be ancillary qubits" and the constructor comment that says "for
a non-ancillary quantum register" to state that the layout can store both
ancillary and non-ancillary qubits; mention that the constructor accepts a
QubitType (parameter typeOfQubitsGeneratedForVariable) which is stored in the
inherited field typeOfQubitsStoredInQuantumRegister, and reflect that wire
variables may be marked QubitType::Ancillary and stored here.
In `@python/mqt/syrec/quantum_circuit_simulation_dialog.py`:
- Around line 637-646: The current call to
QuantumCircuitSimulationDialog._set_enabled_state_of_simulation_run_execution_control_btn_in_tab_based_on_sim_run_execution_mode
temporarily re-enables the RUN_SINGLE button based on rowCount, but a subsequent
QListView selectionChanged handler
(set_enabled_state_of_simulation_run_execution_controls_in_tab_widget_based_on_sim_run_selection_status)
is expected to fire and reconcile the final state after
set_enabled_state_of_simulation_run_execution_controls_in_tab_widget first
disables all controls; add an inline comment next to this call explaining that
the selectionChanged signal will run after this point and will determine the
final button state (mentioning the involved handlers:
set_enabled_state_of_simulation_run_execution_controls_in_tab_widget and
set_enabled_state_of_simulation_run_execution_controls_in_tab_widget_based_on_sim_run_selection_status
and the QListView selectionChanged), so future readers know the double-write is
intentional and reliant on signal ordering.
In `@test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp`:
- Around line 189-191: The assertion allows equality (ASSERT_LE) because
largestQubitIndexInInputState is inputState.size() - 1, but the error message
prints an exclusive upper bound "[0, X)"; update the message to reflect the
inclusive upper bound so it matches the check: change the range text in the
ASSERT_LE failure message for largestNonAncillaryQubitIndexInLookup /
largestQubitIndexInInputState to use an inclusive upper bracket (e.g. "[0, " <<
std::to_string(largestQubitIndexInInputState) << "]") or otherwise print the
inputState.size() as an exclusive bound consistently; ensure the message and the
ASSERT_LE on largestNonAncillaryQubitIndexInLookup remain consistent.
---
Outside diff comments:
In `@python/mqt/syrec/quantum_circuit_simulation_dialog.py`:
- Around line 1147-1148: The type annotations for the methods
set_default_simulation_run_modification_buttons_enabled_state and
_clear_simulation_run_list_and_backing_model should use QtWidgets.QWidget (not
QtWidgets.QTabWidget) because all call sites pass QWidget instances and the code
only uses QWidget methods like objectName() and findChild(); update the function
signatures and any associated parameter annotations to QtWidgets.QWidget so the
types match callers and the used methods are valid for the annotated type.
test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp
Outdated
Show resolved
Hide resolved
…instead of ignoring .clang-tidy file
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/mqt/syrec/quantum_circuit_simulation_dialog.py (2)
696-699:⚠️ Potential issue | 🟡 MinorTooltip text is stale: says "data qubits" but expected size is now
num_qubits.Line 698–699 states that qubit values must match "the number of data qubits of the synthesized quantum computation (i.e. equal to the number of non-ancillary qubits)." However,
_expected_input_output_state_sizeis now set tonum_qubits(line 113), which includes ancillary qubits. Users following this tooltip would provide incorrectly sized states, causing validation failures during import.Proposed fix
Qubit values must be defined as strings containing '0' or '1' characters with the total number of qubits in a - state definition matching the number of data qubits of the synthesized quantum computation (i.e. equal to the number of non-ancillary qubits). + state definition matching the total number of qubits of the synthesized quantum computation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/mqt/syrec/quantum_circuit_simulation_dialog.py` around lines 696 - 699, The tooltip text is stale: it references "data qubits" and "non-ancillary qubits" but the validation uses _expected_input_output_state_size which is set from num_qubits; update the tooltip string in quantum_circuit_simulation_dialog.py (near the list item that mentions qubit values) to state that the string length must equal num_qubits (the total number of qubits, including ancilla), and remove the "data/non-ancillary" wording so it matches the behavior of _expected_input_output_state_size and avoids confusing users during import.
773-786:⚠️ Potential issue | 🟡 MinorUpdate tooltip documentation to reflect actual state size expectations.
The tooltip at lines 698-699 incorrectly states that qubit values should match "the number of data qubits of the synthesized quantum computation (i.e. equal to the number of non-ancillary qubits)." However,
_expected_input_output_state_sizeis initialized tonum_qubits(line 113), which includes all qubits (data + ancillary). The tooltip should clarify that state definitions must account for all qubits, not only data qubits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/mqt/syrec/quantum_circuit_simulation_dialog.py` around lines 773 - 786, The tooltip text currently claims state strings must match "the number of data qubits" but the UI actually expects strings of length _expected_input_output_state_size (initialized from num_qubits), which includes ancillary qubits; update the tooltip/message to state that input/output state definitions must span all qubits (data + ancillary) and reference the actual size variable (e.g. use _expected_input_output_state_size or _annotatable_quantum_computation.num_qubits) so the wording matches behavior in functions like handle_open_and_start_all_input_states_generator_dialog and any validation that compares lengths to _expected_input_output_state_size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/mqt/syrec/quantum_circuit_simulation_dialog.py`:
- Around line 696-699: The tooltip text is stale: it references "data qubits"
and "non-ancillary qubits" but the validation uses
_expected_input_output_state_size which is set from num_qubits; update the
tooltip string in quantum_circuit_simulation_dialog.py (near the list item that
mentions qubit values) to state that the string length must equal num_qubits
(the total number of qubits, including ancilla), and remove the
"data/non-ancillary" wording so it matches the behavior of
_expected_input_output_state_size and avoids confusing users during import.
- Around line 773-786: The tooltip text currently claims state strings must
match "the number of data qubits" but the UI actually expects strings of length
_expected_input_output_state_size (initialized from num_qubits), which includes
ancillary qubits; update the tooltip/message to state that input/output state
definitions must span all qubits (data + ancillary) and reference the actual
size variable (e.g. use _expected_input_output_state_size or
_annotatable_quantum_computation.num_qubits) so the wording matches behavior in
functions like handle_open_and_start_all_input_states_generator_dialog and any
validation that compares lengths to _expected_input_output_state_size.
---
Duplicate comments:
In `@include/core/annotatable_quantum_computation.hpp`:
- Around line 434-445: The constructor comment for
QuantumRegisterForVariableLayout is stale: it says "non-ancillary quantum
register" but the constructor accepts any QubitType (including Ancillary);
update the doc for QuantumRegisterForVariableLayout to remove the
"non-ancillary" wording and state that the constructor creates a variable layout
for a quantum register of the specified QubitType (referencing QubitType) so the
description matches the class-level description and current behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp (1)
88-105:⚠️ Potential issue | 🟡 MinorUpdate class documentation: JSON test data encodes all qubits, not just non-ancillary ones.
The comment at lines 53–54 states: "only the non-ancillary qubit values need to be defined in the input and output state." This is inaccurate. Actual test data (e.g.,
test_sim_production_assignStatement.json) shows that the JSON"in"and"out"strings contain binary digits for all qubit positions from 0 togetNqubits()-1, including ancillary qubits.Update the documentation to clarify the actual format: the state strings must encode every qubit in the computation, and the assertion will compare only the non-ancillary qubit positions against the expected output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp` around lines 88 - 105, Update the misleading comment/documentation around the loop that parses JSON test vectors to state that the JSON "in"/"out" strings encode every qubit position from 0..getNqubits()-1 (including ancillary qubits), not only non-ancillary ones; clarify that generateNonAncillaryQubitsLookup(annotatableQuantumComputation) is used to determine which positions are compared and that input/output strings (accessed via jsonDataOfTestCase, jsonKeyForSimulationRuns and jsonKeyForStringifiedBinaryInputState/jsonKeyForStringifiedBinaryOutputState) must therefore contain bits for all qubits while the assertion assertSimulationResultForStateMatchesExpectedOne only checks the positions in the nonAncillaryQubitsLookup.
🤖 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/mqt/syrec/quantum_circuit_simulation_dialog.py`:
- Line 113: The change makes _expected_input_output_state_size equal the total
num_qubits which breaks older JSON imports whose state bit-strings used the
previous non-ancillary width; update start_import(expected_input_state_size=…)
to detect state strings whose length differs from
_expected_input_output_state_size and, if they match the older width, accept
them by adjusting the expected_input_state_size to len(state_string) (or
pad/expand as appropriate) and emit a clear warning/log entry explaining the
migration; ensure the logic references _expected_input_output_state_size and
start_import so the compatibility check and warning are applied during import.
- Line 1249: There is a typo in the inline comment that currently reads "To tab
changed signal is emitted..." — update that comment to "The tab changed signal
is emitted..." (replace "To" with "The") in the comment near the tab-change
handling code (the comment containing "To tab changed signal is emitted after
the tab already changed thus the QWidget.currentWidget() will return the already
switched to tab widget while we want to reset the selection") so the wording is
correct and clear.
- Around line 1300-1303: Replace the bare if/else with a structural match on
curr_selected_sim_run_exec_mode (using SimulationRunExecutionMode) and explicit
cases: case SimulationRunExecutionMode.RUN_SINGLE: call
targeted_btn.setEnabled(enabled_state_of_targeted_btn_in_single_sim_run_execution_mode);
case _: call
targeted_btn.setEnabled(enabled_state_of_targeted_btn_in_other_execution_modes)
and then call assert_never(curr_selected_sim_run_exec_mode) in the wildcard
branch to ensure exhaustiveness; keep the same variable names
(curr_selected_sim_run_exec_mode, targeted_btn,
enabled_state_of_targeted_btn_in_single_sim_run_execution_mode,
enabled_state_of_targeted_btn_in_other_execution_modes) so static analysis
catches any future enum additions.
---
Outside diff comments:
In `@test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp`:
- Around line 88-105: Update the misleading comment/documentation around the
loop that parses JSON test vectors to state that the JSON "in"/"out" strings
encode every qubit position from 0..getNqubits()-1 (including ancillary qubits),
not only non-ancillary ones; clarify that
generateNonAncillaryQubitsLookup(annotatableQuantumComputation) is used to
determine which positions are compared and that input/output strings (accessed
via jsonDataOfTestCase, jsonKeyForSimulationRuns and
jsonKeyForStringifiedBinaryInputState/jsonKeyForStringifiedBinaryOutputState)
must therefore contain bits for all qubits while the assertion
assertSimulationResultForStateMatchesExpectedOne only checks the positions in
the nonAncillaryQubitsLookup.
---
Duplicate comments:
In `@test/unittests/simulation/fixtures/base_simulation_test_fixture.hpp`:
- Around line 177-196: No changes required: the function
assertSimulationResultForStateMatchesExpectedOne already correctly validates
sizes, handles empty cases, checks bounds using ASSERT_LE for
largestNonAncillaryQubitIndexInLookup vs largestQubitIndexInInputState, and
compares expected vs actual only for qubits in nonAncillaryQubitsLookup with
informative diagnostic messages; leave
syrec::assertSimulationResultForStateMatchesExpectedOne as-is and mark the
review approved.
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
2 participants
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
Prior to this PR, the qubits and thus the quantum register generated for a SyReC variable of type
wirewas only marked as garbage but due to the assumption that these qubits are initialized with the value '0' (see section 2.1 of the RevLib specification) the could also be marked as ancillary ones. With this PR:wireare now marked as ancillary.syrec-editornow correctly consider "gaps" between data qubit indices (introduced by local SyReC module variables of typestate).syrec-editornot correctly considering the selected simulation run execution mode.Excluding the
.clang-tidyfile from the pre-committyposcheck was necessary due to a false-positive typo in the name of theoptin.core.EnumCastOutOfRangecheck being detected, autocorrected and in turn disabling the added check.Fixes #541
Checklist: