🐛 Increased tolerance to avoid undetected degeneracy and added forbidden BDL types in wire detection

[Drewniok]

Description

This PR increases the tolerance used in groundstate_from_simulation_results to extract the ground states from simulation results. Previously, it failed to detect the degeneracy of the ground state in some cases, leading to "weird" operational domains. This should now be fixed.

Example:
This operational domain should be empty, which is only the case if the degeneracy of the ground state is correctly detected. As said, this should be fixed now.

Checklist:

• The pull request only contains commits that are related to it.
• I have added appropriate tests and documentation.
• I have added a changelog entry.
• 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.

[marcelwa]

Thanks for the fix! 🙏

Slightly off-topic, but thinking about the function, I wonder if we should refactor it into a member of sidb_simulation_result called groundstate for more intuitive usability. What do you think, @Drewniok and @wlambooy?

[codecov]

Codecov Report

Attention: Patch coverage is 97.05882% with 4 lines in your changes missing coverage. Please review.

Project coverage is 98.36%. Comparing base (15b53ec) to head (8691e3c).

Files with missing lines	Patch %	Lines
...on/algorithms/simulation/sidb/detect_bdl_wires.hpp	84.61%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #685   +/-   ##
=======================================
  Coverage   98.36%   98.36%           
=======================================
  Files         254      253    -1     
  Lines       41399    41484   +85     
  Branches     1878     1883    +5     
=======================================
+ Hits        40722    40806   +84     
- Misses        677      678    +1     

Files with missing lines	Coverage Δ
...on/algorithms/simulation/sidb/defect_influence.hpp	96.51% <100.00%> (ø)
...ion/algorithms/simulation/sidb/is_ground_state.hpp	100.00% <100.00%> (ø)
...tion/algorithms/simulation/sidb/is_operational.hpp	92.02% <100.00%> (-0.03%)	⬇️
...orithms/simulation/sidb/sidb_simulation_result.hpp	100.00% <100.00%> (ø)
...est/algorithms/simulation/sidb/clustercomplete.cpp	99.68% <100.00%> (ø)
...lation/sidb/exhaustive_ground_state_simulation.cpp	100.00% <100.00%> (ø)
.../algorithms/simulation/sidb/operational_domain.cpp	100.00% <100.00%> (ø)
test/algorithms/simulation/sidb/quickexact.cpp	100.00% <ø> (ø)
test/algorithms/simulation/sidb/quicksim.cpp	100.00% <100.00%> (ø)
...orithms/simulation/sidb/sidb_simulation_result.cpp	100.00% <100.00%> (ø)
... and 2 more

... and 3 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 15b53ec...8691e3c. Read the comment docs.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

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

[Drewniok]

Thanks for the fix! 🙏

Slightly off-topic, but thinking about the function, I wonder if we should refactor it into a member of sidb_simulation_result called groundstate for more intuitive usability. What do you think, @Drewniok and @wlambooy?

That's a great idea! I like it!

[github-actions]

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

[github-actions]

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

[wlambooy]

Thanks for the fix! 🙏

Slightly off-topic, but thinking about the function, I wonder if we should refactor it into a member of sidb_simulation_result called groundstate for more intuitive usability. What do you think, @Drewniok and @wlambooy?

I think that is an excellent idea! Since there is always the possibility of getting degenerate groundstates, we should respect that in the function name, i.e.: groundstates.

[marcelwa]

Thanks for the fix! 🙏

Slightly off-topic, but thinking about the function, I wonder if we should refactor it into a member of sidb_simulation_result called groundstate for more intuitive usability. What do you think, @Drewniok and @wlambooy?

I think that is an excellent idea! Since there is always the possibility of getting degenerate groundstates, we should respect that in the function name, i.e.: groundstates.

Good call! Would it make sense then to parameterize the function to allow degeneracy or not? Or have two separate functions for that? I'm also okay with having a single one simply called groundstates. I'm merely asking for your preference since you're going to use the API the most.

[Drewniok]

Thanks for the fix! 🙏
Slightly off-topic, but thinking about the function, I wonder if we should refactor it into a member of sidb_simulation_result called groundstate for more intuitive usability. What do you think, @Drewniok and @wlambooy?

I think that is an excellent idea! Since there is always the possibility of getting degenerate groundstates, we should respect that in the function name, i.e.: groundstates.

Good call! Would it make sense then to parameterize the function to allow degeneracy or not? Or have two separate functions for that? I'm also okay with having a single one simply called groundstates. I'm merely asking for your preference since you're going to use the API the most.

Personally, I do not see a reason for parameterizing it.

[github-actions]

clang-tidy made some suggestions

[marcelwa]

Maybe I'm missing something here. If you filter wires by a certain type, why do you need to specify another type that is prohibited? Wouldn't any type != type be automatically prohibited by definition?

[Drewniok]

Yes, I understand that this is not great and confusing. Thanks for bringing this up!
I think we should solve this differently and use a different name for the input. The idea is that forbidden_type can be used to remove BDL pairs of the given type from the wire. This is needed in the specific case of a BDL wire with input and output BDL pairs (typically the BDL wires of our gates have either input or output BDL wires and not both).

[marcelwa]

The fact that this seems necessary hints at a design flaw somewhere. Maybe calling the filter function twice to remove all the dots you want gone wouldn't be so bad.

[Drewniok]

hmm, what do you think about the new version.