bqth29 / simulated-bifurcation-algorithm

Python CPU/GPU implementation of the Simulated Bifurcation (SB) algorithm to solve quadratic optimization problems (QUBO, Ising, TSP, optimal asset allocations for a portfolio, etc.).
MIT License
112 stars 26 forks source link

Remove `polynomial` and refactor `core` submodules #78

Open bqth29 opened 2 months ago

bqth29 commented 2 months ago

💬 Pull Request Description

The polynomial was becoming redundant and less relevant as other libraries like SymPy allow polynomial manipulation. As it is a bit out of the scope of this package, it makes sense removing it.

QuadraticPolynomials can still be instanciated from SymPy's Polys or tensors/arrays, but they no longer inherit from Polynomial which means that all the data coherence checks are now carried in the __init__ method of the QuadraticPolynomial class.

This cleaning was carried out alongside a refactoring of the core module that received new responsabilities. For a better alignment of the Ising and QuadraticPolynomial classes, two main changes were introduced:

  1. all the parameters of the optimization functions from Ising, QuadraticPolynomial and the main simulated_bifurcation module are now keyword-only, which replaces PR #53
  2. the optimization methods of QuadraticPolynomial as well as the to_ising method now require a dtype and a device to separate the model's dtype (which can be any int or float dtype) and the computation dtype which must either be torch.float32 or torch.float64; this replaces PR #64 and fixes #41

As reminder, here is what was concluded in #64 " _Based on #41, a reflexion was carried out regarding the difference between the models' dtype and the computation dtype. Because the oscillators in the SB backend are in [-1, 1], the backend computation dtype must be a float. Besides, some key PyTorch methods are not available for float16 so only float32 and float64 are considered. Thus, the core Ising model which is used by the SB optimizer can only be defined with float32 (default) or float64 dtype, any other dtype will raise a ValueError. However, QuadraticPolynomials can still be of any dtype. When such a polynomial is converted to an Ising model, a new dtype argument must be passed to indicate the dtype of the Ising model that will be generated. When calling QuadraticPolynomial::optimize, QuadraticPolynomial::maximize or QuadraticPolynomial::minimize, a dtype argument must also be provided to indicate the backend dtype (for equivalent Ising model and thus SB optimizer computations). Once the optimal spins are retrieved by the polynomial at the end of the optimization and converted to integer values according to the optimization domain, the tensors are converted to the polynomial's dtype. When passing the dtype parameter to the sb.maximize/sb.minimize functions, the dtype will be used as the QuadraticPolynomial's dtype and as the optimization dtype. If not provided, float32 will be used. We concluded that two dtype parameters are not useful for sb.maximize and sb.minimize since the model is only used for optimization and not retrieved in any manner. Thus, using the computation dtype as the model's dtype is acceptable. If users really want to create the model with a specific dtype, they can first build it with build_model and then call one of three optimization methods. Finally, for QuadraticPolynomials, if the dtype and/or device parameters are set to None, the default dtype and/or device of PyTorch will be used._ "

Finally, the domain argument of the optimization functions no longer has a default value.

✔️ Check list

Before you open the pull request, make sure the following requirements are met.

🚀 New features

None.

🐞 Bug fixes

When an Ising model was turned into a single tensor using the as_simulated_bifurcation_tensor method while it had linear terms, Ising.J was passed into the block matrix instead of its null-diagonal and symmetrized version. This was fixed.

📣 Supplementary information

The instances of QuadraticPolynomial now have 4 key properties:

Note that either the QuadraticPolynomial is initialized with tensors and then the Poly is created, or inversely, it is initialized from a Poly and the coefficient tensors are generated afterwards.

Parametrized tests were introduced to cover the core module. They allow the tests to be run using different dtypes and devices (if the computer has a CUDA GPU). Draft PR #61 was closed because this PR replaces it.

GPU tests are to be considered as a good practice (see #75) and should be added systematically from now on.

⚠️ Breaking changes

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 99.74425% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.03%. Comparing base (a9ab8bb) to head (c31a2cb).

Files with missing lines Patch % Lines
tests/models/test_knapsack.py 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #78 +/- ## =========================================== - Coverage 100.00% 97.03% -2.97% =========================================== Files 36 32 -4 Lines 1600 1449 -151 =========================================== - Hits 1600 1406 -194 - Misses 0 43 +43 ``` | [Flag](https://app.codecov.io/gh/bqth29/simulated-bifurcation-algorithm/pull/78/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Thomas+Bouquet) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/bqth29/simulated-bifurcation-algorithm/pull/78/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Thomas+Bouquet) | `97.03% <99.74%> (-2.97%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Thomas+Bouquet#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.