Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

💥 Introducing ClusterComplete: Exact SiDB Logic Simulation with well over 50 DBs #390

Open
wants to merge 636 commits into
base: main
Choose a base branch
from

Conversation

wlambooy
Copy link
Collaborator

@wlambooy wlambooy commented Mar 7, 2024

Description

This rather extensive PR introduces the groundbreaking ClusterComplete exact simulator to fiction. It relies on the concept of "State Space Pruning in a Cluster Hierarchy", which is demonstrated for the first time with the advent of this simulator as a proof of concept. Jan Drewniok's idea of "Physically Informed Space Pruning" is generalised over charge states, and, above all, the relevant electrostatic potential equations are lifted to higher order to enable reasoning in a cluster hiercharchy. This reasoning allows the Ground State Space algorithm to prune multisets of charge states at any point in the cluster hierarchy where it gets the chance, which is enabled by analysis of potential bounds.

The SiDB cluster hierarchy header file technology/sidb_cluster_hierarchy.hpp is now heavily documented, though it does not have associated tests for all the functions and structures used by Ground State Space. Indirectly, they are well tested, however, since the performance of Ground State Space, and, in turn, ClusterComplete are extremely sensitive to any bugs or inconsistencies in this code. Also, since the methodology of the simulation and pre-simulation are translated directly from a generic theory, no room is left for edge cases, except perhaps the empty layout, and the fact that simulation in base 2 can return 0 physically valid layouts. Both of these have a corresponding test

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have created/adjusted the Python bindings for any new or updated functionality.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@wlambooy
Copy link
Collaborator Author

wlambooy commented Mar 7, 2024

The merge brach 'main' in clustercomplete messes with my changed to charge_distribution_surface. This needs to be undone.

Also: the CI has no way of passing at the moment since ALGLIB is not installed. I suppose the following should be added to the workflows:

$ git clone https://github.com/S-Dafarra/alglib-cmake.git
$ cd alglib-cmake
$ mkdir build && cd build
$ cmake ..
$ make
$ [sudo] make install

(clone in the libs folder, the CMakeLists.txt there that was edited in this PR does the rest)

@wlambooy
Copy link
Collaborator Author

wlambooy commented Mar 8, 2024

Latest commit reverts a small change (a bug fix in is_ground_state) that apparently make the TTS tests fail. This is unrelated to this PR, so I moved it to Issue #392 .

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 98.33333% with 23 lines in your changes missing coverage. Please review.

Project coverage is 98.40%. Comparing base (7ba71fb) to head (cf46f74).

Files with missing lines Patch % Lines
.../algorithms/simulation/sidb/ground_state_space.cpp 95.65% 9 Missing ⚠️
...orithms/simulation/sidb/sidb_simulation_engine.hpp 71.42% 6 Missing ⚠️
...st/algorithms/simulation/sidb/time_to_solution.cpp 84.21% 3 Missing ⚠️
...ion/algorithms/simulation/sidb/clustercomplete.hpp 99.27% 2 Missing ⚠️
...lgorithms/simulation/sidb/critical_temperature.hpp 86.66% 2 Missing ⚠️
...lude/fiction/technology/sidb_cluster_hierarchy.hpp 99.57% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #390      +/-   ##
==========================================
+ Coverage   98.12%   98.40%   +0.28%     
==========================================
  Files         235      242       +7     
  Lines       36046    39672    +3626     
  Branches     1754     1815      +61     
==========================================
+ Hits        35370    39041    +3671     
+ Misses        675      631      -44     
+ Partials        1        0       -1     
Files with missing lines Coverage Δ
.../sidb/equivalence_check_for_simulation_results.hpp 100.00% <100.00%> (ø)
...lation/sidb/exhaustive_ground_state_simulation.hpp 94.73% <ø> (ø)
.../algorithms/simulation/sidb/ground_state_space.hpp 100.00% <100.00%> (ø)
...tion/algorithms/simulation/sidb/is_operational.hpp 92.10% <100.00%> (-0.21%) ⬇️
.../fiction/algorithms/simulation/sidb/quickexact.hpp 100.00% <ø> (ø)
...hms/simulation/sidb/sidb_simulation_parameters.hpp 100.00% <ø> (ø)
...on/algorithms/simulation/sidb/time_to_solution.hpp 100.00% <100.00%> (ø)
...fiction/technology/charge_distribution_surface.hpp 99.84% <100.00%> (-0.01%) ⬇️
include/fiction/technology/sidb_charge_state.hpp 96.66% <100.00%> (+4.77%) ⬆️
include/fiction/utils/hash.hpp 100.00% <ø> (ø)
... and 13 more

... and 185 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ba71fb...cf46f74. Read the comment docs.

@wlambooy
Copy link
Collaborator Author

wlambooy commented Mar 8, 2024

It does stop 🥳 Windows CI now sees ALGLIB

@wlambooy
Copy link
Collaborator Author

wlambooy commented Mar 8, 2024

Operational Domain fails, inherited from the ❌ on the main branch at the moment

@wlambooy
Copy link
Collaborator Author

wlambooy commented Mar 8, 2024

It seems that CodeQL does not find ALGLIB, so at the moment it will inspect empty files where the ALGLIB functionality is.

Also there seems to be something wrong with the linker for the Windows CI wrt ALGLIB. For one Windows CI, any file that indirectly includes sidb_cluster_hierarchy fails.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
include/fiction/technology/sidb_charge_state.hpp Outdated Show resolved Hide resolved
@marcelwa marcelwa added the enhancement New feature or request label Mar 8, 2024
@marcelwa marcelwa assigned marcelwa and wlambooy and unassigned marcelwa Mar 8, 2024
@marcelwa marcelwa requested review from marcelwa and Drewniok March 8, 2024 19:34
@wlambooy
Copy link
Collaborator Author

wlambooy commented Mar 8, 2024

Thank you Clang-Tidy for the motivation to generalise the charge state iterators. Implemented it with a nice physical intuition:

enum class sidb_state_iter_dir
{
    TO_VALENCE_BAND,
    TO_CONDUCTANCE_BAND
};

Copy link
Contributor

github-actions bot commented Mar 8, 2024

clang-tidy review says "All clean, LGTM! 👍"

2 similar comments
Copy link
Contributor

github-actions bot commented Mar 8, 2024

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

github-actions bot commented Mar 9, 2024

clang-tidy review says "All clean, LGTM! 👍"

docs/cli.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

test/benchmark/simulation.cpp Show resolved Hide resolved
test/benchmark/simulation.cpp Show resolved Hide resolved
test/benchmark/simulation.cpp Show resolved Hide resolved
test/benchmark/simulation.cpp Show resolved Hide resolved
test/benchmark/simulation.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

test/technology/sidb_cluster_hierarchy.cpp Show resolved Hide resolved
@wlambooy
Copy link
Collaborator Author

Excluding the determinism test for ClusterComplete (10000 iterations of the same simulation) from Debug builds fixes the Windows CI

@Drewniok
Copy link
Collaborator

Excluding the determinism test for ClusterComplete (10000 iterations of the same simulation) from Debug builds fixes the Windows CI

maybe we can reduce it to 100?

@wlambooy
Copy link
Collaborator Author

wlambooy commented Jan 14, 2025

Excluding the determinism test for ClusterComplete (10000 iterations of the same simulation) from Debug builds fixes the Windows CI

maybe we can reduce it to 100?

Good suggestion, implemented:

#ifdef NDEBUG
        for (auto i = 0; i < 10000; i++)
#else
        for (auto i = 0; i < 100; i++)
#endif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants