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

Remove polynomial and refactor core submodules #78

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

Conversation

bqth29
Copy link
Owner

@bqth29 bqth29 commented Sep 28, 2024

💬 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 Use keyword-only parameters for backend optimization functions + do not use default value for optimization domain #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 Force backend computation dtype to float #64 and fixes SB Optimizer computation dtype v. Model dtype #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.

  • The code matches the styling rules
  • The new code is covered by relevant tests
  • Documentation was added

🚀 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:

  • quadratic: square 2-dimensional tensor that gathers all the quadratic coefficients
  • linear: 1-dimensional tensor that gathers all the linear coefficients
  • bias: 0-dimensional tensor that contains the bias of the polynomial
  • poly: SymPy polynomial representation of the QuadraticPolynomial (stored for an easier visualization, manipulation)

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

  • Defining optimization parameters as keyword-only may have introduced breaking changes for users directly calling the impacted backend classes or functions using a version <= 1.2.1

@bqth29 bqth29 added the refactoring Existing code is refactored (no new feature, no breaking change) label Sep 28, 2024
Copy link

codecov bot commented Sep 28, 2024

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 ⚠️
Additional details and impacted files
@@             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 Coverage Δ
97.03% <99.74%> (-2.97%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bqth29 bqth29 changed the title Remove polynomial submodule Remove polynomial and refactor core submodules Sep 29, 2024
@bqth29 bqth29 added bug Something isn't working feature New feature breaking change A breaking change is brought by this PR labels Sep 29, 2024
@bqth29 bqth29 mentioned this pull request Sep 29, 2024
3 tasks
@bqth29 bqth29 added this to the Simulated Bifurcation 2.0 milestone Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A breaking change is brought by this PR bug Something isn't working feature New feature refactoring Existing code is refactored (no new feature, no breaking change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SB Optimizer computation dtype v. Model dtype
1 participant