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

Add QuantumKernel class using terra primitives #437

Merged
merged 121 commits into from
Oct 14, 2022

Conversation

ElePT
Copy link
Collaborator

@ElePT ElePT commented Jul 11, 2022

Summary

This PR presents a reference implementation of a QuantumKernel (original code by @gentinettagian) that leverages the newly introduced primitives (so far, this class is based on the terra interface).

Details and comments

The current design is pending on the approval of the fidelity primitive (see this PR), which is currently blocked by the release of the new primitive sessions.

There are a series of items in the to-do list:

  • Dealing with identical samples in the kernel matrix (see Evaluate quantum kernel matrix elements for identical samples #432)
  • [Update: final names settled] Deciding on class names -> once the new names are set, we can move the code to the kernels directory instead of creating a new primitives submodule
  • [Update: there will be an abstract class] Assessing whether the TrainableQuantumKernel should have a mixin/additional base class (as proposed in the latest version of the code). It makes sense for type-hinting purposes (so that we can check in algorithms whether Kernel is trainable), but in terms of functionality I don't see much advantage.
  • Assessing whether additional batching functionality is needed

@ElePT ElePT changed the base branch from main to primitives July 11, 2022 12:13
qiskit_machine_learning/primitives/kernels/base_kernel.py Outdated Show resolved Hide resolved
qiskit_machine_learning/primitives/kernels/base_kernel.py Outdated Show resolved Hide resolved
r"""
Construct kernel matrix for given data

If y_vec is None, self inner product is calculated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting of variables in docs missing.

qiskit_machine_learning/primitives/kernels/base_kernel.py Outdated Show resolved Hide resolved
qiskit_machine_learning/primitives/kernels/base_kernel.py Outdated Show resolved Hide resolved
return kernel_matrix

def _get_symmetric_kernel_matrix(
self, left_parameters: np.ndarray, right_parameters: np.ndarray, shape: Tuple
Copy link
Contributor

Choose a reason for hiding this comment

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

Tuple type missing.

Comment on lines 141 to 147
kernel_entries = self._fidelity.compute(left_parameters, right_parameters)
kernel_matrix = np.zeros(shape)
index = 0
for i in range(shape[0]):
for j in range(i, shape[1]):
kernel_matrix[i, j] = kernel_entries[index]
index += 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Code duplication with the previous method. Could parametrize the starting point of the inner for loop - 0 vs i.


class TrainableQuantumKernel(QuantumKernel):
"""
Trainable overlap kernel.
Copy link
Contributor

Choose a reason for hiding this comment

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

More info needed.

qiskit_machine_learning/utils/utils.py Outdated Show resolved Hide resolved


@ddt
class TestQuantumKernelClassify(QiskitMachineLearningTestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, a test class should correspond to a class being tested, not a behaviour being tested.

@adekusar-drl
Copy link
Collaborator

I have not looked at the feature maps in some while. The idea of the BlueprintCircuit (what I keep calling template circuit) was that base classes can extend this and invalidate it etc over changes requiring it to be rebuilt. So setting feature_dimension as an indirect to num_qubits may be a better option, but not sure they handle that or not. Either way it comes down to use cases. When we first did Aqua we did it as you are talking about where a new instance needed to be created for each use. As more application stack logic got built out this linear progression down the stack building stuff out as you go needed to be revised. We wanted higher level iteration over the algorithms, be able to build out the algorithm, ie create/config it, for later use by the stack, but often while a user might be able to select a circuit up front the number of qubits was not known. Hence the need to be able to update in that regard. So for me it comes down to how one might expect these to be used going forwards.

feature_dimension does not work with all circuits. I'm going to keep as it is now, but this part to be revised.

@woodsp-ibm woodsp-ibm added this to the 0.5.0 milestone Oct 13, 2022
Comment on lines 99 to 100
elif not isinstance(quantum_kernel, BaseKernel):
raise TypeError("'quantum_kernel' has to be of type None or BaseKernel")
Copy link
Member

Choose a reason for hiding this comment

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

Typically we do not police types like this especially when only one type is specified by the typehint. If you wish to keep it then it might be nice to indicate the failing type in the error message too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed this elif clause.

@@ -340,14 +345,15 @@ def precomputed(self) -> bool:
@precomputed.setter
def precomputed(self, precomputed: bool):
"""Sets the pre-computed kernel flag. If ``True`` is passed then the previous kernel is
cleared. If ``False`` is passed then a new instance of ``QuantumKernel`` is created."""
cleared. If ``False`` is passed then a new instance of
:class:`~qiskit_machine_learning.kernels.BaseKernel` is created."""
Copy link
Member

Choose a reason for hiding this comment

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

Should this be FidelityQuantumKernel - that is a BaseKernel, but is that too abstract that a user needs to know the actual type?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's right, updated.

@woodsp-ibm
Copy link
Member

Looking at the coverage its very good - BaseKernel is a fair bit lower. Error cases and properties seem to get missed often in testing where they end up not being covered.

adekusar-drl and others added 8 commits October 13, 2022 23:18
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
…5.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
@adekusar-drl
Copy link
Collaborator

Looking at the coverage its very good - BaseKernel is a fair bit lower. Error cases and properties seem to get missed often in testing where they end up not being covered.

Added more tests for the base class and for fidelity kernels.

Copy link
Member

@woodsp-ibm woodsp-ibm left a comment

Choose a reason for hiding this comment

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

LGTM!

@adekusar-drl adekusar-drl merged commit 66e0476 into qiskit-community:main Oct 14, 2022
oscar-wallis pushed a commit that referenced this pull request Feb 16, 2024
* Add primitives branch to CI workflow

* added pocs

* init fixed

* more init

* Add qnns PoC

* lint fixed

* Migrate qnns to new branch

* some unittests

* more unittest

* adapted new design without unittests

* removed compute

* removed compute

* Change inputs, assign parameters method name

* docstrings

* working on dict version

* support dictionarys

* Update qiskit_machine_learning/primitives/kernels/base_kernel.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit_machine_learning/primitives/kernels/base_kernel.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Small fixes

* Remove changes from trailing commit

* Update qiskit_machine_learning/utils/utils.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* raise error if not all parameters bound

* typo

* unit tests

* training unit tests

* feature params order fixex

* lint

* Create trainable mixin

* Update qiskit_machine_learning/primitives/kernels/base_kernel.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit_machine_learning/primitives/kernels/base_kernel.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit_machine_learning/primitives/kernels/pseudo_kernel.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Fix fidelity imports (new location)

* Remove pseudo kernel

* evaluate duplicates

* merge

* black

* fix some issues

* update to the recent interfaces

* update tests

* fix fidelity parameterization

* skip some tests

* fix pylint, mypy

* fix copyright

* more tweaks

* rename methods

* refactor

* clean up

* some docstrings

* more tests

* streamline base class

* fix spell

* trainable qk tweaks

* fix trainability

* revert workflows

* revert the notebook

* renamings, refactorings, code review

* code review

* remove commented code

* bump terra requirements

* code review

* code review

* fix pylint

* enforce psd test

* fix docstring

* add reno

* fix documentation reference

* update qsvc, more tests

* update qsvr, even more tests

* fix qsvr tests

* fix lint

* Change Terra requirements to accept rc

* update pegasos

* fix test

* Update qiskit_machine_learning/algorithms/classifiers/pegasos_qsvc.py

Co-authored-by: Gian Gentinetta <31244916+gentinettagian@users.noreply.github.com>

* fix complex eigenvalues

* pass parameters as keywords

* fix trainability, code review

* mixin -> abstract class, keywords

* fix docs

* update docs

* code review

* fix tests, pylint

* update reno

* update reno

* fix reno

* Update qiskit_machine_learning/algorithms/classifiers/qsvc.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update qiskit_machine_learning/algorithms/regressors/qsvr.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update qiskit_machine_learning/kernels/quantum_kernel.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update releasenotes/notes/add-fidelity-quantum-kernel-d40278abb49e19b5.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* code review

* fix tests

* terra 0.22

* more tests

Co-authored-by: Manoel Marques <manoel.marques@ibm.com>
Co-authored-by: Gian Gentinetta <gian.gentinetta@gmx.ch>
Co-authored-by: dlasecki <dal@zurich.ibm.com>
Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Gian Gentinetta <31244916+gentinettagian@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants