💥 Introducing ClusterComplete: Exact SiDB Logic Simulation with well over 50 DBs
#390
Conversation
|
The merge brach 'main' in clustercomplete messes with my changed to 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) |
|
Latest commit reverts a small change (a bug fix in |
|
It does stop 🥳 Windows CI now sees ALGLIB |
|
Operational Domain fails, inherited from the ❌ on the main branch at the moment |
|
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
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
}; |
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
include/fiction/algorithms/simulation/sidb/ground_state_space.hpp
Outdated
Show resolved
Hide resolved
include/fiction/algorithms/simulation/sidb/ground_state_space.hpp
Outdated
Show resolved
Hide resolved
# Conflicts: # .github/workflows/pyfiction-pypi-deployment.yml
…ACT in bindings test)
|
Wow Clang-Tidy started working I see! Any idea what fixed it? I see macOS fails currently because of some unrelated fmt format building issue |
Wow! This is amazing! I don't know why it suddenly works. |
|
Wow Clang-Tidy started working I see! Any idea what fixed it? I see macOS fails currently because of some unrelated fmt format building issue |
Signed-off-by: Marcel Walter <[email protected]>
# Conflicts: # cli/cmd/simulation/opdom.hpp
# Conflicts: # test/benchmark/simulation.cpp
# Conflicts: # test/algorithms/simulation/sidb/critical_temperature.cpp
Signed-off-by: GitHub Actions <[email protected]>
# Conflicts: # bindings/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp
Signed-off-by: GitHub Actions <[email protected]>
There was a problem hiding this comment.
Dear @wlambooy thank you so much for this wonderful addition to fiction! The code is in a remarkable shape. I have a few comments and suggestions here and there, but nothing too extravagant I hope. One thing I noticed was that compiling the new features still results in some warnings. I would appreciate it if you could address those. After submitting this review, I will commit a few edits I made myself. And then I'll have a deeper look into the CIs.
| // Let's figure out what do with this. I think there should be two results, since {0, 1, 1} has a local potential | ||
| // of 0.3199995 when all other SiDBs are NEG, which is their only possible charge state. Without error bounds, this | ||
| // SiDB should be strictly NEG, but error bounds exist to relax bounds, not make them stricter. Therefore, also the | ||
| // NEUT charge state should be accepted since the range is then (0.32 - e, 0.91 + e) = (0.319999, 0.910001). | ||
| // (note that I use local potential values according to Vij = - ...) | ||
| // SECTION("Test case 4") | ||
| // { | ||
| // TestType lyt{}; | ||
| // | ||
| // lyt.assign_cell_type({0, 0, 0}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({0, 1, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({9, 3, 0}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({2, 4, 0}, TestType::cell_type::NORMAL); | ||
| // | ||
| // const clustercomplete_params<cell<TestType>> params{sidb_simulation_parameters{3, -0.32}}; | ||
| // const quickexact_params<TestType> paramsqe{sidb_simulation_parameters{3, -0.32}}; | ||
| // | ||
| // const auto simulation_results = clustercomplete<TestType>(lyt, params); | ||
| // const auto simulation_results2 = quickexact<TestType>(lyt, paramsqe); | ||
| // | ||
| // CHECK(simulation_results.charge_distributions.size() == 1); | ||
| // } |
There was a problem hiding this comment.
What's up with this test case? The comment string indicates there are two possible solutions here. Maybe @Drewniok could also comment on what he thinks is best how do go about it.
| // Same as Test case 4. | ||
| // SECTION("Test case 7") | ||
| // { | ||
| // TestType lyt{}; | ||
| // | ||
| // lyt.assign_cell_type({11, 1, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({15, 1, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({5, 2, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({17, 2, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({20, 3, 0}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({13, 3, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({9, 4, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({2, 5, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({17, 5, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({2, 6, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({9, 6, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({10, 7, 0}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({8, 7, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({17, 9, 0}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({11, 9, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({12, 9, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({5, 10, 0}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({5, 10, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({7, 11, 1}, TestType::cell_type::NORMAL); | ||
| // lyt.assign_cell_type({13, 11, 1}, TestType::cell_type::NORMAL); | ||
| // | ||
| // const sidb_simulation_parameters params{2, -0.32}; | ||
| // | ||
| // sidb_simulation_result<TestType> cc_res = clustercomplete(lyt, | ||
| // clustercomplete_params<cell<TestType>>{params}); | ||
| // | ||
| // std::sort(cc_res.charge_distributions.begin(), cc_res.charge_distributions.end(), | ||
| // [](const auto& lhs, const auto& rhs) { return lhs.get_system_energy() < rhs.get_system_energy(); | ||
| // }); | ||
| // | ||
| // REQUIRE(cc_res.charge_distributions.size() == 1); | ||
| // } |
There was a problem hiding this comment.
Can this be removed then?
| using ccsim_command = clustercomplete_command; | ||
|
|
||
| ALICE_ADD_COMMAND(clustercomplete, "Simulation") | ||
| ALICE_ADD_COMMAND(ccsim, "Simulation") |
There was a problem hiding this comment.
If you want to add a shorthand for your command, you can do so in shortcuts.fs and then in your CLI session use < ../shortcuts.fs.
| {"Iteration steps", | ||
| std::any_cast<uint64_t>(sim_result_100.additional_simulation_parameters.at("iteration_steps"))}, | ||
| {"alpha", std::any_cast<double>(sim_result_100.additional_simulation_parameters.at("alpha"))}}; |
There was a problem hiding this comment.
Aren't these QuickSim stats you're logging here?
| {"Iteration steps", | ||
| std::any_cast<uint64_t>(sim_result_111.additional_simulation_parameters.at("iteration_steps"))}, | ||
| {"alpha", std::any_cast<double>(sim_result_111.additional_simulation_parameters.at("alpha"))}}; |
| #if (FICTION_ALGLIB_ENABLED) | ||
| else if (params.engine == sidb_simulation_engine::CLUSTERCOMPLETE) | ||
| { | ||
| const clustercomplete_params<cell<Lyt>> cc_params{params.simulation_parameters}; | ||
|
|
||
| // All physically valid charge configurations are determined for the given layout (`ClusterComplete` | ||
| // simulation is used to provide 100 % accuracy for the Critical Temperature). | ||
| simulation_results = clustercomplete(layout, cc_params); | ||
| } | ||
| #endif // FICTION_ALGLIB_ENABLED |
There was a problem hiding this comment.
Shall we add an #else case with an assert or something notifying the user that ALGLIB must be installed if ClusterComplete is to be used?
This would then apply to all such cases.
| [[nodiscard]] inline fiction::sidb_simulation_engine get_sim_engine() const noexcept | ||
| { | ||
| if (sim_engine_str == "ClusterComplete") | ||
| { | ||
| return fiction::sidb_simulation_engine::CLUSTERCOMPLETE; | ||
| } | ||
|
|
||
| if (sim_engine_str == "QuickSim") | ||
| { | ||
| return fiction::sidb_simulation_engine::QUICKSIM; | ||
| } | ||
|
|
||
| return fiction::sidb_simulation_engine::QUICKEXACT; | ||
| } |
There was a problem hiding this comment.
As mentioned above, this function could be replaced with a global one selecting engines.
| LOWER = 0, | ||
| UPPER |
There was a problem hiding this comment.
Could you add docstrings to the enum values?
There was a problem hiding this comment.
Wouldn't this test case break down if you set DEBUG_SIDB_CLUSTER_HIERARCHY due to the explicit usage of phmap::flat_hash_set?
There was a problem hiding this comment.
Please add docstrings to all functions and members (even the private) ones throughout this file. It will greatly help with maintainability in the future.
Signed-off-by: GitHub Actions <[email protected]>
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.hppis 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 testChecklist: