QNNCircuit circuit added to wrap feature map and ansatz #569

david-alber · 2023-03-07T17:59:24Z

Closes #505

BlueprintCircuit QNNCircuit added to wrap feature map and ansatz.
This circuit simplifies the composition of a feature map and an ansatz.
The circuit can be constructed by providing either the number of qubits, a feature map or an ansatz.
If only the number of qubits is provided the RealAmplitudes ansatz ZZFeatureMap
are used. If the number of qubits is 1 the ZFeatureMapis used.
If only a feature map is provided, the RealAmplitudesansatz with the
corresponding number of qubits is used. If only an ansatz is provided the ZZFeatureMap
with the corresponding number of qubits is used.

CLAassistant · 2023-03-07T18:42:36Z

All committers have signed the CLA.

coveralls · 2023-03-08T18:23:06Z

Pull Request Test Coverage Report for Build 4616784943

49 of 54 (90.74%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.001%) to 90.479%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit_machine_learning/circuit/library/qnn_circuit.py	46	51	90.2%

Totals
Change from base Build 4538457628:	-0.001%
Covered Lines:	3478
Relevant Lines:	3844

💛 - Coveralls

woodsp-ibm

I left some comments more open to discussion so that @adekusar-drl can add his thoughts as to how to proceed.

I wonder whether given such an entity whether instead of this, as per the issue.

qnn_qc = QNNCircuit(num_inputs, ZZFeatureMap, RealAmplitudes)

qnn = EstimatorQNN(
    circuit=qnn_qc,
    input_params=qnn_qc.input_parameters,
    weight_params=qnn_qc.weight_parameters,
)

I should be simply able to do

qnn_qc = QNNCircuit(num_inputs, ZZFeatureMap, RealAmplitudes)

qnn = EstimatorQNN(
    circuit=qnn_qc
)

where EstimatorQNN detects the QNNCircuit type and can get the parameters itself rather than the user having to pass them over. Of course such could be the subject of a different PR leaving this focused on simply provisioning the new circuit.

qiskit_machine_learning/circuit/library/qnn_circuit.py

qiskit_machine_learning/circuit/library/__init__.py

qiskit_machine_learning/circuit/library/qnn_circuit.py

adekusar-drl · 2023-03-09T13:31:37Z

Of course such could be the subject of a different PR leaving this focused on simply provisioning the new circuit.

As discussed with @david-alber, this will be another good first issue.

qiskit_machine_learning/circuit/library/qnn_circuit.py

woodsp-ibm · 2023-03-10T19:34:06Z

qiskit_machine_learning/circuit/library/qnn_circuit.py

+            num_qubits:  Number of qubits.
+            feature_map: A feature map.
+            ansatz:      An ansatz.


num_qubits: Optional number of qubits. If not supplied defaults from sizes of feature_map and ansatz. If both feature map and ansatz are left as default then it must be provided. feature_map: An optional feature map. If not provided defaults to ZZFeatureMap, or ZFeatureMap if num qubits is determined to be . ansatz: An optional ansatz. If not provided defaults to RealAmplitudes. if not provided.

Can we put some description on each of the parameters, say as above. The class docstring has a more wordy description overall but hopefully this summarizes enough.

woodsp-ibm · 2023-03-10T19:46:54Z

I am not sure on this. Maybe we should sort out the circuits in the constructor too, like it was before. and maybe when things are set. With it being delayed to build some methods have different behavior on whether it has been built or not - where the lazy building arguably ought to be transparent.

Some code with examples - as it states some things need to be uncommented to run - they throw exceptions.

from qiskit.circuit.library import ZFeatureMap, ZZFeatureMap, RealAmplitudes, EfficientSU2
from qiskit_machine_learning.circuit.library import QNNCircuit

# The blocks of code commented out fail e.g dereferencing None in the first
# 2 lines

def main():
  #qc = QNNCircuit()
  #qc.input_parameters

  #qc = QNNCircuit(2)
  #qc.weight_parameters

  #qc = QNNCircuit(ansatz=RealAmplitudes)
  #print(qc)

  # For this one it does not fail in constructor despite passing both, only
  # later when its built to be printed. Should we check earlier.
  #
  #qc = QNNCircuit(feature_map=ZZFeatureMap(2), ansatz=RealAmplitudes())
  #print(qc)   # This causes it to be built so it can be printed.

  # Change num qubits behavior
  #
  qc = QNNCircuit(feature_map=ZZFeatureMap(2), ansatz=RealAmplitudes(2))
  print(qc)
  qc.num_qubits = 3
  print(qc.num_qubits) # Its 3 as expected but...
  print(qc.input_parameters) # ...we still get the old num qubits parameters here
  print(qc)
  print(qc.input_parameters) # We do not get a new value until here after its built again.
                             # Setting num_qubits into the contained feature_map and ansazt
                             # too in the setter would likely sort this to behave as expected
  #
  # Contrast that to this below where changing num_qubits changes parameters.
  #
  qc=ZZFeatureMap(2) # RealAmplitudes (2, reps=1)
  print(qc.parameters)
  qc.num_qubits = 3
  print(qc.parameters)

  # Updating say feature map before and after building differs (no num qubits)
  #
  qc = QNNCircuit(feature_map=ZZFeatureMap(2), ansatz=RealAmplitudes(2))
  #qc.feature_map = ZFeatureMap(3) # This fails since num_qubits has not been set yet
  print(qc)
  qc.feature_map = ZFeatureMap(3) # Is adjusted to existing num qubits now been set
  print(qc)
  qc.ansatz=EfficientSU2(reps=1)
  print(qc)
  #
  # This is ok since we set num_qubits upfront - adjusts new ansatz to 3
  #
  qc = QNNCircuit(num_qubits=3, feature_map=ZZFeatureMap(2), ansatz=RealAmplitudes(2))
  qc.ansatz = EfficientSU2(6, reps=1)
  print(qc)


if __name__ == "__main__":
    main()

print vars working version of QNNCircuit adding unittests for QNNCircuit add feature releas note for QNNCircuit black, lint, compyright add CNNCircuit to wrap feature map and ansatz correct spelling mistakes spell check resolve pylint unused variable 'circuit' update type hints, copyright & correct release notes formating derive property config. whenever circuit is build, add respective tests and detailed release note Update qiskit_machine_learning/circuit/library/qnn_circuit.py typo correction: adding missing space. Co-authored-by: Steve Wood <[email protected]> extended argument description valid parametersets at all time, adjust tests & docu. respectively

woodsp-ibm · 2023-03-24T17:03:42Z

As a request can you please avoid force pushing -normally one can see delta changes here between commits, which is very helpful when reviewing as things are progressed. Force pushing loses that.

david-alber · 2023-03-24T17:16:07Z

@woodsp-ibm: I apologize for that! Lesson learned. Will avoid force pushing in the future.
Sorry!

adekusar-drl · 2023-03-31T07:54:06Z

@woodsp-ibm Can you please take a look? Since I'm working with David on the PR, I don't think I should approve it.

woodsp-ibm · 2023-03-31T16:53:19Z

qiskit_machine_learning/circuit/library/qnn_circuit.py

+                         if num_qubits is determined to be 1.
+            ansatz:      An ansatz. Optional if num_qubits or feature_map is provided, otherwise
+                         required. If not provided defaults to ``RealAmplitudes``.
+


Does this state the behavior? I am wondering if a note here, something like this, possibly referring to the above more lengthy class doc, nearer the args would better alert the user and summarize things more clearly.

Although all parameters default to None at least one parameter MUST be provided, so that the number of qubits (non-zero) can be determined from it, when the instance is created.

If more than one parameter is passed, then if num_qubits is provided any feature map and/or ansatz supplied will be overidden to the same value, as long as the respective circuit supports updating its num_qubits. If num_qubits is not provided the feature_map and ansatz MUST be set to the SAME number of qubits.

About knowing the num qubits upfront at creation time. The whole notion of a BlueprintCircuit was it was a template where the num_qubits might not be known upfront and could be later set. Now it can be set later, but cannot be an unknown at the creation point it seems. I know if a FeatureMap instance from what we have today are passed its feature_dimension has to be set (which ends up for what we have so far as same number of qubits). That the Feature Map is a blueprint circuit, where we need to know the feature dim, which ends up defining the num qubits. I guess its comparable.

About knowing the num qubits upfront at creation time. The whole notion of a BlueprintCircuit was it was a template where the num_qubits might not be known upfront and could be later set. Now it can be set later, but cannot be an unknown at the creation point it seems. I know if a FeatureMap instance from what we have today are passed its feature_dimension has to be set (which ends up for what we have so far as same number of qubits). That the Feature Map is a blueprint circuit, where we need to know the feature dim, which ends up defining the num qubits. I guess its comparable.

I don't really get the point here. At least, one of the parameters must be set and the number of qubits is derived from the value of this parameter. Later, the number of qubits can be updated and, I think, this is the desired behavior.

QNNCircuit extends BlueprintCircuit (which is why I mentioned it in second sentence). So QNNCircuit is a BlueprintCircuit. But it does not behave that way like RealAmplitudes(), another Blueprint circuit, which I can create without defining num qubits. Its more like ZZFeatureMap though the parameter there is feature_dimension (which has a correspondance though to num qubits) - hence I mentioned at the end maybe its comparable to that in some respects. As long as things are clearly stated for its behavior (which is the text preceding that to try to summarize things, as for me the class docstring is kinda lengthy in spelling it all out) it seems fine.

I see, thanks for the clarification. Yes, you are right, in that sense the circuit resembles ZZFeatureMap.

woodsp-ibm · 2023-03-31T16:58:44Z

qiskit_machine_learning/circuit/library/qnn_circuit.py

+    ) -> None:
+        """
+        Args:
+            num_qubits:  Number of qubits. Optional if feature_map or ansatz is provided, otherwise


Can we say here this is a positive integer. Since it needs to be that if passed. 0 errors out with our check, a negative errors out when we try and build ZZFeatureMap with that value.

woodsp-ibm · 2023-03-31T17:08:35Z

qiskit_machine_learning/circuit/library/qnn_circuit.py

+
+        Returns:
+            The composed feature map and the ansatz circuit.
+        """


It might be nice to have Raises here to denote this may error at construction time to re-inforce the above note. I saw ValueError is say num_qubits was -2, and QiskitMachineLearningError from our validation check.

woodsp-ibm · 2023-03-31T17:40:22Z

qiskit_machine_learning/circuit/library/qnn_circuit.py

+        return self._ansatz
+
+    @ansatz.setter
+    def ansatz(self, ansatz: QuantumCircuit) -> None:


these properties are adjusted respectively

I think this should be explained as to what this adjustment actually is.

As far as I can tell the QNNCircuit is updated to now have an overall num_qubits the same as the ansatz being passed to the setter (assuming QNNCircuits FeatureMap can be adjusted).

qc = QNNCircuit(feature_map=ZZFeatureMap(2), ansatz=RealAmplitudes(2)) qc.ansatz=EfficientSU2(3, reps=1) print(qc)

Circuit is 3 qubits wide now having started out as 2.

In other words the circuit is adjusted to the ansatz being passed in, rather than the other way around.

Since it's the former, the ansatz passed has to have a positive number for num_qubits. The following, just passing in the ansatz without that being set fails (as it does not adjust the ansatz to the present circuit size)

qc = QNNCircuit(feature_map=ZZFeatureMap(2), ansatz=RealAmplitudes(2)) qc.ansatz=EfficientSU2(reps=1)

if you want that you can achieve that net result by setting it explicitly to match yourself

qc = QNNCircuit(feature_map=ZZFeatureMap(2), ansatz=RealAmplitudes(2)) qc.ansatz=EfficientSU2(qc.num_qubits, reps=1)

The FeatureMap setter has the same behavior as far as I can tell and its docstring should be improved in a similar manner.

adekusar-drl · 2023-03-31T19:17:22Z

qiskit_machine_learning/circuit/library/qnn_circuit.py

+    If only the number of qubits is provided the ``RealAmplitudes`` ansatz ``ZZFeatureMap``
+    are used. If the number of qubits is 1 the ``ZFeatureMap`` is used.


The can suggestion can break the limit of the number of characters per line.

Suggested change

If only the number of qubits is provided the ``RealAmplitudes`` ansatz ``ZZFeatureMap``

are used. If the number of qubits is 1 the ``ZFeatureMap`` is used.

If only the number of qubits is provided the ``RealAmplitudes`` ansatz and the ``ZZFeatureMap`` feature map are used. If the number of qubits is 1 the ``ZFeatureMap`` is used.

woodsp-ibm · 2023-04-04T18:29:20Z

qiskit_machine_learning/circuit/library/qnn_circuit.py

+            QiskitMachineLearningError: If the correct number of qubits cannot be derived form
+            the provided input arguments.
+            ValueError: If the number of qubits is invalid.
+            AttributeError: If the feature_map or ansatz is invalid.


I don't know about listing AttributeError here. Basically you would only get this if you provide some object that does not conform to the typehint, i.e. not a QuantumCircuit. This is most likely what happens when it assumes its a QuantumCircuit and tries to access fields like num_qubits, that should be in the object, but are not. Python then raises this error. Arguably all bets are off if you pass the wrong types. I personally would remove this and the tests that pass some non QuantumCircuit object wherever this has been added. (Otherwise arguably we should have this all over the place!!)

qiskit_machine_learning/circuit/library/qnn_circuit.py

woodsp-ibm

LGTM. Thanks for the contribution to Qiskit Machine Learning!

david-alber · 2023-04-05T19:30:36Z

This was fun. Thanks for the great feedback 😀.

* local env set up print vars working version of QNNCircuit adding unittests for QNNCircuit add feature releas note for QNNCircuit black, lint, compyright add CNNCircuit to wrap feature map and ansatz correct spelling mistakes spell check resolve pylint unused variable 'circuit' update type hints, copyright & correct release notes formating derive property config. whenever circuit is build, add respective tests and detailed release note Update qiskit_machine_learning/circuit/library/qnn_circuit.py typo correction: adding missing space. Co-authored-by: Steve Wood <[email protected]> extended argument description valid parametersets at all time, adjust tests & docu. respectively * more explicit documentation and test cases for invalid construction * add class ref. in docs., rm attribute-, value-error tests and docs. --------- Co-authored-by: Anton Dekusar <[email protected]>

david-alber requested review from stefan-woerner, manoelmarques, woodsp-ibm and adekusar-drl as code owners March 7, 2023 17:59

woodsp-ibm reviewed Mar 8, 2023

View reviewed changes

adekusar-drl changed the title ~~CNNCircuit circuit added to wrap feature map and ansatz~~ QNNCircuit circuit added to wrap feature map and ansatz Mar 9, 2023

woodsp-ibm reviewed Mar 10, 2023

View reviewed changes

woodsp-ibm added this to the 0.7.0 milestone Mar 21, 2023

david-alber force-pushed the fixissue505branch branch from ed39341 to d149dde Compare March 24, 2023 15:50

woodsp-ibm removed the request for review from manoelmarques March 24, 2023 18:18

Merge branch 'main' into fixissue505branch

0be8c42

woodsp-ibm reviewed Mar 31, 2023

View reviewed changes

adekusar-drl reviewed Mar 31, 2023

View reviewed changes

more explicit documentation and test cases for invalid construction

36033f1

woodsp-ibm reviewed Apr 4, 2023

View reviewed changes

qiskit_machine_learning/circuit/library/qnn_circuit.py Outdated Show resolved Hide resolved

woodsp-ibm reviewed Apr 4, 2023

View reviewed changes

qiskit_machine_learning/circuit/library/qnn_circuit.py Outdated Show resolved Hide resolved

add class ref. in docs., rm attribute-, value-error tests and docs.

87929e4

woodsp-ibm approved these changes Apr 5, 2023

View reviewed changes

adekusar-drl merged commit 1283036 into qiskit-community:main Apr 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QNNCircuit circuit added to wrap feature map and ansatz #569

QNNCircuit circuit added to wrap feature map and ansatz #569

david-alber commented Mar 7, 2023

CLAassistant commented Mar 7, 2023 •

edited

Loading

coveralls commented Mar 8, 2023 •

edited

Loading

woodsp-ibm left a comment

adekusar-drl commented Mar 9, 2023

woodsp-ibm Mar 10, 2023

woodsp-ibm commented Mar 10, 2023

woodsp-ibm commented Mar 24, 2023

david-alber commented Mar 24, 2023

adekusar-drl commented Mar 31, 2023

woodsp-ibm Mar 31, 2023

adekusar-drl Mar 31, 2023

woodsp-ibm Mar 31, 2023

adekusar-drl Mar 31, 2023

woodsp-ibm Mar 31, 2023

woodsp-ibm Mar 31, 2023

woodsp-ibm Mar 31, 2023

adekusar-drl Mar 31, 2023

woodsp-ibm Apr 4, 2023 •

edited

Loading

woodsp-ibm left a comment

david-alber commented Apr 5, 2023

		If only the number of qubits is provided the ``RealAmplitudes`` ansatz ``ZZFeatureMap``
		are used. If the number of qubits is 1 the ``ZFeatureMap`` is used.

QNNCircuit circuit added to wrap feature map and ansatz #569

QNNCircuit circuit added to wrap feature map and ansatz #569

Conversation

david-alber commented Mar 7, 2023

CLAassistant commented Mar 7, 2023 • edited Loading

coveralls commented Mar 8, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4616784943

💛 - Coveralls

woodsp-ibm left a comment

Choose a reason for hiding this comment

adekusar-drl commented Mar 9, 2023

Choose a reason for hiding this comment

woodsp-ibm commented Mar 10, 2023

woodsp-ibm commented Mar 24, 2023

david-alber commented Mar 24, 2023

adekusar-drl commented Mar 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodsp-ibm Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

woodsp-ibm left a comment

Choose a reason for hiding this comment

david-alber commented Apr 5, 2023

CLAassistant commented Mar 7, 2023 •

edited

Loading

coveralls commented Mar 8, 2023 •

edited

Loading

woodsp-ibm Apr 4, 2023 •

edited

Loading