✨ Sorting designed SiDB gates - #552
Open
wlambooy wants to merge 264 commits into
Open
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ned_gates_sorting # Conflicts: # include/fiction/algorithms/physical_design/design_sidb_gates.hpp
Collaborator
|
@wlambooy is this PR still under active development, or can it be closed? |
Collaborator
Author
I actually started merging it with main yesterday (big task in this case). It is a good starting point for the advanced circuit design I'm getting into. I'll let you know if this PR will stay or if it will be part of a different one. |
Signed-off-by: GitHub Actions <actions@github.com>
…ned_gates_sorting
Signed-off-by: GitHub Actions <actions@github.com>
…ned_gates_sorting
Collaborator
Author
|
The changes in this PR introduce some functionality that is fundamental to the new circuit design project that I'm working on now. By merging these changes in main already soon, we prevent having some massive PR later |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
==========================================
- Coverage 98.16% 98.12% -0.04%
==========================================
Files 261 263 +2
Lines 40105 40417 +312
Branches 2110 2130 +20
==========================================
+ Hits 39370 39661 +291
- Misses 735 756 +21
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Signed-off-by: Willem Lambooy <35612535+wlambooy@users.noreply.github.com>
Drewniok
reviewed
Jun 3, 2025
Collaborator
There was a problem hiding this comment.
could you make sure to use #if (FICTION_ALGLIB_ENABLED) where CC is used.
Collaborator
Author
There was a problem hiding this comment.
I actually do not see any uses. Did I miss something?
Collaborator
Author
There was a problem hiding this comment.
I double checked it again. There is only one removed mention of CLUSTERCOMPLETE. Please check if your comment still holds
# Conflicts: # bindings/mnt/pyfiction/include/pyfiction/algorithms/physical_design/design_sidb_gates.hpp # bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp # bindings/mnt/pyfiction/test/algorithms/physical_design/test_design_sidb_gates.py # bindings/mnt/pyfiction/test/algorithms/simulation/sidb/test_is_operational.py # include/fiction/algorithms/physical_design/design_sidb_gates.hpp # include/fiction/algorithms/simulation/sidb/is_operational.hpp
Signed-off-by: GitHub Actions <actions@github.com>
…ned_gates_sorting
…ned_gates_sorting
…ned_gates_sorting
# Conflicts: # bindings/mnt/pyfiction/include/pyfiction/documentation.hpp # bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp # bindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cpp # bindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/sidb/is_operational.cpp # bindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/sidb/sidb_simulation_parameters.cpp # bindings/mnt/pyfiction/src/pyfiction/inout/write_qca_layout.cpp # bindings/mnt/pyfiction/src/pyfiction/layouts/cartesian_layout.cpp # bindings/mnt/pyfiction/src/pyfiction/layouts/coordinates.cpp # bindings/mnt/pyfiction/src/pyfiction/layouts/gate_level_layout.cpp # bindings/mnt/pyfiction/src/pyfiction/layouts/hexagonal_layout.cpp # bindings/mnt/pyfiction/src/pyfiction/layouts/obstruction_layout.cpp # bindings/mnt/pyfiction/src/pyfiction/layouts/shifted_cartesian_layout.cpp # experiments/standard_cell_design/quickcell_vs_automatic_exhaustive_2_input.cpp # include/fiction/algorithms/physical_design/design_sidb_gates.hpp # include/fiction/technology/sidb_cluster_hierarchy.hpp # libs/CMakeLists.txt # test/algorithms/physical_design/design_sidb_gates.cpp
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
💤 Files with no reviewable changes (7)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis PR refactors SiDB operational assessments into structured results, separates kink and positive-charge conditions, generalizes gate-design parameters and execution modes, adds gate-ordering comparators, updates Python bindings and documentation, and migrates dependent C++, experiment, benchmark, and test code. ChangesSiDB API and gate-design refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
include/fiction/algorithms/simulation/sidb/is_operational.hpp (1)
235-251: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
extract_simulation_results_per_input()copies rather than moves.The method is
constand the loopfor (operational_assessment_for_input assessment : assessment_per_input.value())iterates by value, so eachsidb_simulation_result<Lyt>(a vector of charge-distribution surfaces) is deep-copied before the innerstd::move. This contradicts the "through moves" docstring and can be costly for every operational gate during post-design sorting. Since callers bind the assessment asconst operational_assessment<Lyt>&, a true move requires exposing a non-const overload (e.g.&&-qualified) and iterating by reference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/fiction/algorithms/simulation/sidb/is_operational.hpp` around lines 235 - 251, Update extract_simulation_results_per_input so result extraction has a non-const rvalue-qualified path that can move retained simulation results. Iterate assessment_per_input by reference rather than value, move each referenced assessment’s simulation_results into the returned vector, and update callers to invoke this overload on a movable assessment instead of a const reference while preserving the existing assertions and output behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/mnt/pyfiction/__init__.py`:
- Around line 114-119: Re-export design_sidb_gates_stats from the package
initializer: add it to the relevant import list and __all__ alongside the other
design_sidb_gates symbols, preserving the public
mnt.pyfiction.design_sidb_gates_stats API.
In `@bindings/mnt/pyfiction/include/pyfiction/documentation.hpp`:
- Around line 98-99: Update the documentation string for
__doc_fiction_design_sidb_gates_stats_pruning_total_overridden to explicitly
describe the total runtime spent in the pruning phase, distinguishing it from
the overall time_total statistic.
In
`@bindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cpp`:
- Around line 46-47: Update the EXHAUSTIVE enum binding in the
design_sidb_gates_mode definition to use the EXHAUSTIVE-specific documentation
symbol instead of the QUICKCELL documentation symbol; leave the QUICKCELL
binding unchanged.
In
`@bindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/sidb/is_operational.cpp`:
- Line 91: Update the constructor binding’s py::arg name in
operational_assessment_for_input from " op_status" to "op_status", matching the
sibling binding so the normal op_status keyword is accepted.
In `@docs/algorithms/design_sidb_gates.rst`:
- Around line 26-29: Replace the four `.. doxygenenum::` directives in the
Python enum section of `design_sidb_gates.rst` with `.. autoclass::` directives
targeting the corresponding `mnt.pyfiction` enum bindings: both design mode
enums and both termination condition enums, matching the existing Python enum
documentation style.
In `@include/fiction/algorithms/physical_design/design_sidb_gates.hpp`:
- Around line 539-586: Synchronize the shared num_solutions_found counter in
both extract_gate_designs
(include/fiction/algorithms/physical_design/design_sidb_gates.hpp:539-586) and
run_random_design
(include/fiction/algorithms/physical_design/design_sidb_gates.hpp:310-382).
Prefer making the counter std::atomic<uint64_t> and use atomic operations for
increments and termination-guard reads, preserving the existing
maximum-number-of-solutions behavior without data races or overshooting.
In
`@include/fiction/algorithms/simulation/sidb/compare_by_ground_state_isolation.hpp`:
- Around line 177-197: Update average_ground_state_isolation_over_all_inputs to
handle count == 0 before dividing, returning
std::numeric_limits<double>::infinity() when every simulation result has a
single charge distribution; preserve the existing accumulated average for count
greater than zero.
In `@test/algorithms/physical_design/design_sidb_gates.cpp`:
- Around line 357-362: In the QuickCell validation block, update the
first_gate_quickcell binding to read from found_gate_layouts_quickcell rather
than found_gate_layouts_exhaustive. Keep the existing operational-status and
layout-count checks unchanged.
---
Nitpick comments:
In `@include/fiction/algorithms/simulation/sidb/is_operational.hpp`:
- Around line 235-251: Update extract_simulation_results_per_input so result
extraction has a non-const rvalue-qualified path that can move retained
simulation results. Iterate assessment_per_input by reference rather than value,
move each referenced assessment’s simulation_results into the returned vector,
and update callers to invoke this overload on a movable assessment instead of a
const reference while preserving the existing assertions and output behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4614d7ca-92c4-4966-b16c-2c8ced8627b3
📒 Files selected for processing (48)
.readthedocs.ymlbindings/mnt/pyfiction/__init__.pybindings/mnt/pyfiction/include/pyfiction/documentation.hppbindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hppbindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cppbindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/sidb/is_operational.cppbindings/mnt/pyfiction/test/algorithms/physical_design/test_design_sidb_gates.pybindings/mnt/pyfiction/test/algorithms/simulation/sidb/test_is_operational.pybindings/mnt/pyfiction/test/algorithms/simulation/sidb/test_operational_domain.pydocs/algorithms/design_sidb_gates.rstdocs/algorithms/sidb_simulation.rstexperiments/figure_of_merit_analysis/fom_analysis_2_input_1_output.cppexperiments/operational_domain/critical_temperature_domain_bestagon.cppexperiments/operational_domain/operational_domain_bestagon.cppexperiments/operational_domain/operational_domain_bestagon_grid_vs_sketch.cppexperiments/physical_design_with_on_the_fly_gate_design/on_the_fly_sidb_circuit_design_on_defective_surface.cppexperiments/quicktrace/quicktrace_vs_grid_search_new_gates.cppexperiments/sidb_simulation/temperature/critical_temperature_simulation_bestagon.cppexperiments/sidb_simulation/temperature/critical_temperature_simulation_siqad.cppexperiments/standard_cell_design/automatic_exhaustive_gate_design.cppexperiments/standard_cell_design/pruning_vs_simulation_scalability.cppexperiments/standard_cell_design/quickcell_3_input.cppexperiments/standard_cell_design/quickcell_rectangular_gate_library.cppexperiments/standard_cell_design/quickcell_vs_automatic_exhaustive_2_input.cppinclude/fiction/algorithms/physical_design/apply_gate_library.hppinclude/fiction/algorithms/physical_design/compare_designed_sidb_gates.hppinclude/fiction/algorithms/physical_design/design_sidb_gates.hppinclude/fiction/algorithms/physical_design/exact.hppinclude/fiction/algorithms/physical_design/on_the_fly_sidb_circuit_design.hppinclude/fiction/algorithms/simulation/sidb/calculate_energy_and_state_type.hppinclude/fiction/algorithms/simulation/sidb/compare_by_ground_state_isolation.hppinclude/fiction/algorithms/simulation/sidb/critical_temperature.hppinclude/fiction/algorithms/simulation/sidb/defect_influence.hppinclude/fiction/algorithms/simulation/sidb/displacement_robustness_domain.hppinclude/fiction/algorithms/simulation/sidb/is_operational.hppinclude/fiction/algorithms/simulation/sidb/operational_domain.hppinclude/fiction/technology/sidb_on_the_fly_gate_library.hppinclude/fiction/utils/layout_utils.hppinclude/fiction/utils/math_utils.hpptest/algorithms/physical_design/apply_gate_library.cpptest/algorithms/physical_design/design_sidb_gates.cpptest/algorithms/simulation/sidb/critical_temperature.cpptest/algorithms/simulation/sidb/is_operational.cpptest/algorithms/simulation/sidb/operational_domain.cpptest/algorithms/simulation/sidb/operational_domain_ratio.cpptest/algorithms/simulation/sidb/verify_logic_match.cpptest/benchmark/sidb_logic.cpptest/utils/math_utils.cpp
💤 Files with no reviewable changes (1)
- .readthedocs.yml
Comment on lines
+114
to
+119
| design_sidb_gates_params_100, | ||
| design_sidb_gates_params_111, | ||
| design_sidb_gates_params_design_sidb_gates_mode_100, | ||
| design_sidb_gates_params_design_sidb_gates_mode_111, | ||
| design_sidb_gates_params_termination_condition_100, | ||
| design_sidb_gates_params_termination_condition_111, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether design_sidb_gates_stats is still bound in the pybind11 module and whether it's re-exported.
rg -n 'design_sidb_gates_stats' bindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cpp
rg -n 'design_sidb_gates_stats' bindings/mnt/pyfiction/__init__.pyRepository: cda-tum/fiction
Length of output: 1649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the export list around the reported lines and search for any re-export of the stats class.
python3 - <<'PY'
from pathlib import Path
path = Path("bindings/mnt/pyfiction/__init__.py")
lines = path.read_text().splitlines()
for start in (1, 90, 110, 420, 520):
end = min(len(lines), start + 30)
print(f"\n--- {path}:{start}-{end} ---")
for i in range(start, end + 1):
print(f"{i:4}: {lines[i-1]}")
PY
printf '\n--- grep ---\n'
rg -n 'design_sidb_gates_stats|__all__|design_sidb_gates_params_' bindings/mnt/pyfiction/__init__.pyRepository: cda-tum/fiction
Length of output: 7170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm the binding exists in the C++ module and whether the Python package references it anywhere else.
rg -n 'py::class_<fiction::design_sidb_gates_stats>|design_sidb_gates_stats' bindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cpp bindings/mnt/pyfictionRepository: cda-tum/fiction
Length of output: 7597
Re-export design_sidb_gates_stats
bindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cpp still binds design_sidb_gates_stats, but bindings/mnt/pyfiction/__init__.py no longer exposes it. Add it back to the import list and __all__ so mnt.pyfiction.design_sidb_gates_stats remains available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/mnt/pyfiction/__init__.py` around lines 114 - 119, Re-export
design_sidb_gates_stats from the package initializer: add it to the relevant
import list and __all__ alongside the other design_sidb_gates symbols,
preserving the public mnt.pyfiction.design_sidb_gates_stats API.
Comment on lines
+98
to
+99
| inline constexpr auto __doc_fiction_design_sidb_gates_stats_pruning_total_overridden = | ||
| R"doc(The total runtime of the process.)doc"; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify pruning_total docstring to distinguish it from time_total.
Both pruning_total (line 98-99) and time_total (line 101-102) are documented with nearly identical wording ("The total runtime of the process." vs "The total runtime of the SiDB gate design process."). Since pruning_total presumably measures time spent specifically in the pruning phase, the docstring should say so explicitly to avoid confusing the two stats.
📝 Proposed fix
inline constexpr auto __doc_fiction_design_sidb_gates_stats_pruning_total_overridden =
- R"doc(The total runtime of the process.)doc";
+ R"doc(The total runtime spent on pruning non-operational layouts.)doc";📝 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
| inline constexpr auto __doc_fiction_design_sidb_gates_stats_pruning_total_overridden = | |
| R"doc(The total runtime of the process.)doc"; | |
| inline constexpr auto __doc_fiction_design_sidb_gates_stats_pruning_total_overridden = | |
| R"doc(The total runtime spent on pruning non-operational layouts.)doc"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/mnt/pyfiction/include/pyfiction/documentation.hpp` around lines 98 -
99, Update the documentation string for
__doc_fiction_design_sidb_gates_stats_pruning_total_overridden to explicitly
describe the total runtime spent in the pruning phase, distinguishing it from
the overall time_total statistic.
Comment on lines
+46
to
+47
| .value("EXHAUSTIVE", fiction::design_sidb_gates_params<Lyt>::design_sidb_gates_mode::EXHAUSTIVE, | ||
| DOC(fiction_design_sidb_gates_params_design_sidb_gates_mode_QUICKCELL)) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrong docstring reference for EXHAUSTIVE.
The EXHAUSTIVE enum value is bound with DOC(fiction_design_sidb_gates_params_design_sidb_gates_mode_QUICKCELL), a copy-paste of the QUICKCELL docstring. It should reference the EXHAUSTIVE doc.
Proposed fix
.value("EXHAUSTIVE", fiction::design_sidb_gates_params<Lyt>::design_sidb_gates_mode::EXHAUSTIVE,
- DOC(fiction_design_sidb_gates_params_design_sidb_gates_mode_QUICKCELL))
+ DOC(fiction_design_sidb_gates_params_design_sidb_gates_mode_EXHAUSTIVE))📝 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
| .value("EXHAUSTIVE", fiction::design_sidb_gates_params<Lyt>::design_sidb_gates_mode::EXHAUSTIVE, | |
| DOC(fiction_design_sidb_gates_params_design_sidb_gates_mode_QUICKCELL)) | |
| .value("EXHAUSTIVE", fiction::design_sidb_gates_params<Lyt>::design_sidb_gates_mode::EXHAUSTIVE, | |
| DOC(fiction_design_sidb_gates_params_design_sidb_gates_mode_EXHAUSTIVE)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cpp`
around lines 46 - 47, Update the EXHAUSTIVE enum binding in the
design_sidb_gates_mode definition to use the EXHAUSTIVE-specific documentation
symbol instead of the QUICKCELL documentation symbol; leave the QUICKCELL
binding unchanged.
| py::class_<typename fiction::operational_assessment<Lyt>::operational_assessment_for_input>( | ||
| m, fmt::format("operational_assessment_for_input{}", lattice).c_str(), | ||
| DOC(fiction_operational_assessment_operational_assessment_for_input)) | ||
| .def(py::init<fiction::operational_status>(), py::arg(" op_status"), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant binding file around the reported lines.
FILE="bindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/sidb/is_operational.cpp"
wc -l "$FILE"
sed -n '70,120p' "$FILE"
echo
echo "---- registration references ----"
rg -n "operational_assessment|is_operational_impl|simulation_result|sidb_simulation_result|register_.*sidb" bindings/mnt/pyfiction/src -SRepository: cda-tum/fiction
Length of output: 15781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,80p' bindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/sidb/register_sidb_support.cpp
echo
sed -n '1,80p' bindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/sidb/register_sidb_simulation.cpp
echo
sed -n '1,80p' bindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/register_simulation.cppRepository: cda-tum/fiction
Length of output: 2883
Fix the constructor keyword typo
py::arg(" op_status") registers the parameter with a leading space, so operational_assessment_for_input(..., op_status=...) can’t be called with the normal keyword name. Rename it to py::arg("op_status") to match the sibling binding.
Proposed fix
- .def(py::init<fiction::operational_status>(), py::arg(" op_status"),
+ .def(py::init<fiction::operational_status>(), py::arg("op_status"),📝 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
| .def(py::init<fiction::operational_status>(), py::arg(" op_status"), | |
| .def(py::init<fiction::operational_status>(), py::arg("op_status"), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bindings/mnt/pyfiction/src/pyfiction/algorithms/simulation/sidb/is_operational.cpp`
at line 91, Update the constructor binding’s py::arg name in
operational_assessment_for_input from " op_status" to "op_status", matching the
sibling binding so the normal op_status keyword is accepted.
Comment on lines
+26
to
+29
| .. doxygenenum:: mnt.pyfiction.design_sidb_gates_params_design_sidb_gates_mode_100 | ||
| .. doxygenenum:: mnt.pyfiction.design_sidb_gates_params_design_sidb_gates_mode_111 | ||
| .. doxygenenum:: mnt.pyfiction.design_sidb_gates_params_termination_condition_100 | ||
| .. doxygenenum:: mnt.pyfiction.design_sidb_gates_params_termination_condition_111 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline docs/algorithms/design_sidb_gates.rst --view expanded || true
printf '\n== Relevant lines ==\n'
sed -n '1,120p' docs/algorithms/design_sidb_gates.rst | cat -n
printf '\n== Search for autoclass/doxygenenum in docs ==\n'
rg -n --glob '*.rst' 'autoclass::|doxygenenum::' docs | sed -n '1,200p'
printf '\n== Search for these exact enum symbols ==\n'
rg -n 'design_sidb_gates_params_(design_sidb_gates_mode_100|design_sidb_gates_mode_111|termination_condition_100|termination_condition_111)' -S . | sed -n '1,200p'Repository: cda-tum/fiction
Length of output: 21283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Nearby Python enum docs examples ==\n'
rg -n --glob '*.rst' 'Python tab|autoclass:: .*enum|autoclass:: mnt\.pyfiction' docs | sed -n '1,220p'
printf '\n== Files referencing design_sidb_gates ==\n'
rg -n 'design_sidb_gates' docs include src python -S | sed -n '1,220p'Repository: cda-tum/fiction
Length of output: 19102
Use autodoc for the Python enum bindings. The Python tab in docs/algorithms/design_sidb_gates.rst should use .. autoclass:: for these four mnt.pyfiction enums, matching the rest of the Python enum docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/algorithms/design_sidb_gates.rst` around lines 26 - 29, Replace the four
`.. doxygenenum::` directives in the Python enum section of
`design_sidb_gates.rst` with `.. autoclass::` directives targeting the
corresponding `mnt.pyfiction` enum bindings: both design mode enums and both
termination condition enums, matching the existing Python enum documentation
style.
Comment on lines
+539
to
+586
| ++num_solutions_found; | ||
| } | ||
| else | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // pruning was already conducted above. Hence, SIMULATION_ONLY is chosen. | ||
| params.operational_params.strategy_to_analyze_operational_status = | ||
| is_operational_params::operational_analysis_strategy::SIMULATION_ONLY; | ||
| const std::scoped_lock lock_vector{mutex_to_protect_designed_gate_layouts}; | ||
|
|
||
| designed_gate_layouts.gate_layouts.emplace_back(std::move(layout_with_added_cells)); | ||
|
|
||
| if (const auto [status, sim_calls] = is_operational(candidate, truth_table, params.operational_params, | ||
| input_bdl_wires, output_bdl_wires); | ||
| status == operational_status::OPERATIONAL) | ||
| #if (PROGRESS_BARS) | ||
| if (params.termination_cond == design_sidb_gates_params<Lyt>::termination_condition::OBTAINED_N_SOLUTIONS && | ||
| num_solutions_found < params.maximum_number_of_solutions) | ||
| { | ||
| // Lock and update shared resources | ||
| { | ||
| const std::scoped_lock lock{mutex_to_protect_gate_designs}; | ||
| gate_layouts.push_back(candidate); | ||
| } | ||
| gate_design_found = true; // Notify all threads that a solution has been found | ||
| // update the progress bar | ||
| bar(num_solutions_found); | ||
| } | ||
| #endif | ||
| }; | ||
|
|
||
| std::vector<std::thread> threads{}; | ||
| threads.reserve(num_threads); | ||
|
|
||
| for (std::size_t i = 0; i < num_threads; ++i) | ||
| { | ||
| threads.emplace_back( | ||
| [this, i, chunk_size, &gate_candidates, &check_operational_status, &gate_design_found]() | ||
| [this, i, chunk_size, &candidate_combinations, &add_combination_to_layout_and_check_operation, | ||
| &num_solutions_found, &max_number_of_solutions | ||
| #if (PROGRESS_BARS) | ||
| , | ||
| &bar | ||
| #endif | ||
| ] | ||
| { | ||
| const std::size_t start_index = i * chunk_size; | ||
| const std::size_t end_index = std::min(start_index + chunk_size, gate_candidates.size()); | ||
| const std::size_t end_index = std::min(start_index + chunk_size, candidate_combinations.size()); | ||
|
|
||
| for (std::size_t j = start_index; j < end_index; ++j) | ||
| { | ||
| if (gate_design_found && | ||
| (params.termination_cond == | ||
| design_sidb_gates_params<cell<Lyt>>::termination_condition::AFTER_FIRST_SOLUTION)) | ||
| if (params.termination_cond == | ||
| design_sidb_gates_params<Lyt>::termination_condition::OBTAINED_N_SOLUTIONS && | ||
| num_solutions_found >= max_number_of_solutions) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| check_operational_status(gate_candidates[j]); | ||
| add_combination_to_layout_and_check_operation(candidate_combinations[j]); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Non-atomic num_solutions_found counter is raced by worker threads in two design paths. Both multithreaded designers share a plain uint64_t num_solutions_found that one thread mutates while others read the loop/stop guard outside the mutex, which is a data race (UB) and permits overshooting maximum_number_of_solutions. Make the counter std::atomic<uint64_t> (or perform the increment and the guard check under the same lock).
include/fiction/algorithms/physical_design/design_sidb_gates.hpp#L539-L586:extract_gate_designsincrementsnum_solutions_found(Line 539) before acquiring the lock at Line 546 and reads it in the thread loop at Line 581 without a lock — synchronize both.include/fiction/algorithms/physical_design/design_sidb_gates.hpp#L310-L382:run_random_designreads the loop guard at Line 334 without the mutex while incrementing under lock at Line 382 — synchronize the guard read.
📍 Affects 1 file
include/fiction/algorithms/physical_design/design_sidb_gates.hpp#L539-L586(this comment)include/fiction/algorithms/physical_design/design_sidb_gates.hpp#L310-L382
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/fiction/algorithms/physical_design/design_sidb_gates.hpp` around
lines 539 - 586, Synchronize the shared num_solutions_found counter in both
extract_gate_designs
(include/fiction/algorithms/physical_design/design_sidb_gates.hpp:539-586) and
run_random_design
(include/fiction/algorithms/physical_design/design_sidb_gates.hpp:310-382).
Prefer making the counter std::atomic<uint64_t> and use atomic operations for
increments and termination-guard reads, preserving the existing
maximum-number-of-solutions behavior without data races or overshooting.
Comment on lines
+177
to
+197
| [[nodiscard]] double | ||
| average_ground_state_isolation_over_all_inputs(const sidb_gate_design& gate_design) const noexcept | ||
| { | ||
| uint64_t count = 0; | ||
|
|
||
| double accumulated_ground_state_isolation = 0.0; | ||
|
|
||
| for (const auto& sim_res : gate_design.simulation_results_per_input) | ||
| { | ||
| if (sim_res.charge_distributions.size() == 1) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| accumulated_ground_state_isolation += detail::get_ground_state_isolation(sim_res); | ||
|
|
||
| ++count; | ||
| } | ||
|
|
||
| return accumulated_ground_state_isolation / static_cast<double>(count); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against division by zero in average isolation.
If every input's simulation_results_per_input has a single charge distribution, the continue at Line 188 skips all iterations, leaving count == 0, and the return computes 0.0 / 0.0 = NaN. Because this value is used as a sort key inside std::sort, a NaN comparator result violates strict-weak-ordering and is undefined behavior. Return std::numeric_limits<double>::infinity() (consistent with the single-distribution case in get_ground_state_isolation) when count == 0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@include/fiction/algorithms/simulation/sidb/compare_by_ground_state_isolation.hpp`
around lines 177 - 197, Update average_ground_state_isolation_over_all_inputs to
handle count == 0 before dividing, returning
std::numeric_limits<double>::infinity() when every simulation result has a
single charge distribution; preserve the existing accumulated average for count
greater than zero.
Comment on lines
+357
to
+362
| REQUIRE(found_gate_layouts_quickcell.size() == 1); | ||
| const auto& first_gate_quickcell = found_gate_layouts_exhaustive.front(); | ||
| CHECK( | ||
| is_operational(first_gate_quickcell, std::vector<tt>{create_and_tt()}, params.operational_params).status == | ||
| operational_status::OPERATIONAL); | ||
| CHECK(design_gates_stats_quickcell.number_of_layouts == 1); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wrong container: first_gate_quickcell reads from the exhaustive result.
first_gate_quickcell is bound to found_gate_layouts_exhaustive.front(), so the QuickCell branch validates the exhaustive layout instead of the QuickCell one. This defeats the purpose of the section.
🐛 Proposed fix
- const auto& first_gate_quickcell = found_gate_layouts_exhaustive.front();
+ const auto& first_gate_quickcell = found_gate_layouts_quickcell.front();📝 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
| REQUIRE(found_gate_layouts_quickcell.size() == 1); | |
| const auto& first_gate_quickcell = found_gate_layouts_exhaustive.front(); | |
| CHECK( | |
| is_operational(first_gate_quickcell, std::vector<tt>{create_and_tt()}, params.operational_params).status == | |
| operational_status::OPERATIONAL); | |
| CHECK(design_gates_stats_quickcell.number_of_layouts == 1); | |
| REQUIRE(found_gate_layouts_quickcell.size() == 1); | |
| const auto& first_gate_quickcell = found_gate_layouts_quickcell.front(); | |
| CHECK( | |
| is_operational(first_gate_quickcell, std::vector<tt>{create_and_tt()}, params.operational_params).status == | |
| operational_status::OPERATIONAL); | |
| CHECK(design_gates_stats_quickcell.number_of_layouts == 1); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/algorithms/physical_design/design_sidb_gates.cpp` around lines 357 -
362, In the QuickCell validation block, update the first_gate_quickcell binding
to read from found_gate_layouts_quickcell rather than
found_gate_layouts_exhaustive. Keep the existing operational-status and
layout-count checks unchanged.
- Remove unused fiction/traits.hpp and fiction/layouts/coordinates.hpp includes from the pyfiction design_sidb_gates binding. - Remove unused fiction/traits.hpp and lorina/lorina.hpp includes from the on-the-fly SiDB circuit design experiment; include lorina/verilog.hpp directly since that is what provides lorina::read_verilog. - Annotate unchecked std::optional accesses with NOLINT, matching the existing convention used elsewhere in the codebase. - Remove unused <filesystem> and <iostream> includes from the operational_domain_bestagon experiment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@experiments/physical_design_with_on_the_fly_gate_design/on_the_fly_sidb_circuit_design_on_defective_surface.cpp`:
- Around line 163-170: Update the result-handling block around st.gate_layout,
mockturtle::miter, and mockturtle::equivalence_checking to validate each
optional before dereferencing it. Replace the assert-only protection for eq with
runtime guards that safely handle missing miter or equivalence results, and
invoke sidb_circuits_with_defects only when both values are available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8443a6c9-f244-4ee8-a9b5-e22086643321
📒 Files selected for processing (3)
bindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cppexperiments/operational_domain/operational_domain_bestagon.cppexperiments/physical_design_with_on_the_fly_gate_design/on_the_fly_sidb_circuit_design_on_defective_surface.cpp
💤 Files with no reviewable changes (2)
- experiments/operational_domain/operational_domain_bestagon.cpp
- bindings/mnt/pyfiction/src/pyfiction/algorithms/physical_design/design_sidb_gates.cpp
Comment on lines
+163
to
+170
| const auto& gate_layout = st.gate_layout.value(); // NOLINT(bugprone-unchecked-optional-access) | ||
| const auto miter = mockturtle::miter<mockturtle::klut_network>(mapped_network, gate_layout); | ||
| const auto eq = mockturtle::equivalence_checking(*miter); // NOLINT(bugprone-unchecked-optional-access) | ||
| assert(eq.has_value()); | ||
| const auto eq_result = *eq; // NOLINT(bugprone-unchecked-optional-access) | ||
|
|
||
| sidb_circuits_with_defects(benchmark, mockturtle::to_seconds(st.time_total), | ||
| st.exact_stats.num_aspect_ratios, *eq, result.num_cells()); | ||
| st.exact_stats.num_aspect_ratios, eq_result, result.num_cells()); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target snippet =="
sed -n '130,190p' experiments/physical_design_with_on_the_fly_gate_design/on_the_fly_sidb_circuit_design_on_defective_surface.cpp
echo
echo "== search for miter/equivalence_checking usages =="
rg -n "equivalence_checking|miter<mockturtle::klut_network>|gate_layout\.value\(|gate_layout" experiments -S
echo
echo "== locate mockturtle references in repo =="
rg -n "namespace mockturtle|equivalence_checking\(" -S .Repository: cda-tum/fiction
Length of output: 25651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the exact symbols and relevant type hints in the repository
rg -n "equivalence_checking\(|miter<|struct .*gate_layout|gate_layout\.value\(" -S .
# If a local copy of mockturtle headers exists in the repo, inspect the symbol definitions
fd -a "mockturtle" .Repository: cda-tum/fiction
Length of output: 18915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' experiments/physical_design_with_on_the_fly_gate_design/on_the_fly_sidb_circuit_design_on_defective_surface.cppRepository: cda-tum/fiction
Length of output: 9257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,150p' include/fiction/algorithms/verification/equivalence_checking.hpp
echo
echo "== miter wrapper definition =="
sed -n '1,220p' include/fiction/utils/mockturtle_utils.hpp
echo
echo "== exact target context in a similar experiment =="
sed -n '160,190p' experiments/defect_aware_physical_design/defect_aware_physical_design.cppRepository: cda-tum/fiction
Length of output: 6464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the repository's wrappers and the concrete miter/equivalence pattern used elsewhere.
sed -n '1,180p' include/fiction/algorithms/verification/equivalence_checking.hpp
echo
sed -n '1,140p' include/fiction/utils/mockturtle_utils.hpp
echo
sed -n '170,190p' experiments/defect_aware_physical_design/defect_aware_physical_design.cppRepository: cda-tum/fiction
Length of output: 8409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,140p' include/fiction/algorithms/verification/equivalence_checking.hpp
echo
sed -n '1,120p' include/fiction/utils/mockturtle_utils.hpp
echo
sed -n '170,190p' experiments/defect_aware_physical_design/defect_aware_physical_design.cppRepository: cda-tum/fiction
Length of output: 5252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "gate_layout|on_the_fly_sidb_circuit_design_on_defective_surface_stats|on_the_fly_circuit_design_on_defective_surface_stats" include experiments -S
echo
sed -n '1,260p' include/fiction/algorithms/physical_design/on_the_fly_sidb_circuit_design.hppRepository: cda-tum/fiction
Length of output: 19054
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the implementation file and inspect the public contract around stats and return values.
fd -a "on_the_fly_sidb_circuit_design.hpp" include
fd -a "on_the_fly_sidb_circuit_design*.hpp" include
echo
sed -n '1,260p' include/fiction/algorithms/physical_design/on_the_fly_sidb_circuit_design.hppRepository: cda-tum/fiction
Length of output: 11114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "gate_layout" include/fiction -SRepository: cda-tum/fiction
Length of output: 6612
Guard the optional miter/equivalence results before dereferencing. st.gate_layout is populated by the design routine, but *miter and *eq still rely on optional values; assert(eq.has_value()) won’t protect release builds.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 168-168: Found an exit path from function with non-void return type that has missing return statement
(missingReturn)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@experiments/physical_design_with_on_the_fly_gate_design/on_the_fly_sidb_circuit_design_on_defective_surface.cpp`
around lines 163 - 170, Update the result-handling block around st.gate_layout,
mockturtle::miter, and mockturtle::equivalence_checking to validate each
optional before dereferencing it. Replace the assert-only protection for eq with
runtime guards that safely handle missing miter or equivalence results, and
invoke sidb_circuits_with_defects only when both values are available.
Source: Linters/SAST tools
Proactively scanned every file touched by this PR's true diff against main with clang-tidy's misc-include-cleaner and bugprone-unchecked-optional-access checks, since CI only surfaces a partial batch of findings per run. - Remove unused includes: <thread>, <atomic>, <mockturtle/views/color_view.hpp>, and <kitty/dynamic_truth_table.hpp> from exact.hpp; unused fiction/traits.hpp from several standard_cell_design experiments and two test files. - Annotate unchecked std::optional accesses with NOLINT, matching the existing convention, in exact.hpp, compare_designed_sidb_gates.hpp, is_operational.hpp, compare_by_ground_state_isolation.hpp, and critical_temperature.hpp. - Add missing direct includes for Catch2 macros used in several test files that previously relied on transitive includes, and add <cstddef> for std::size_t in math_utils.cpp. - Refactor a chain of repeated `.value()` accesses in the is_operational test into named references to keep the NOLINT annotations stable across clang-format reflows.
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
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 PR introduces an extension to
is_operationalanddesign_sidb_gates. The former is now able to return the simulation results that certify the statusOPERATIONAL, which may then be used by the extension to the latter, which uses the simulation results to return the designed layouts in a specific ordering. This PR implements a simple heuristic for this ordering, namely it prefers the layouts for the the ground state isolation is large, ie., the energetic gap between the ground state and the first excited state. Specifically, it sorts by the minimum ground state isolation for each input.To use this new feature when designing SiDB gates, make sure that all combinations are enumerated, and set the
post_design_processparameter toPREFER_ENERGETICALLY_ISOLATED_GROUND_STATES.Checklist:
Summary by CodeRabbit