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

Added getter/setter, exception handling for OpflowQNN, CircuitQNN #79

Merged

Conversation

stephenmurray2
Copy link
Contributor

@stephenmurray2 stephenmurray2 commented Apr 30, 2021

Summary

This PR closes #75:

  • Added getter and setter to OpflowQNN
  • In CircuitQNN, raises errors in methods for which a quantum_instance is required

Details and comments

My first PR in this repo, still figuring things out :)

@CLAassistant
Copy link

CLAassistant commented Apr 30, 2021

CLA assistant check
All committers have signed the CLA.

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.

Welcome and thanks for looking at this. I made some comments in regards of the aspects I know about this code.

@@ -80,6 +80,16 @@ def __init__(self, operator: OperatorBase,
super().__init__(len(self._input_params), len(self._weight_params),
sparse=False, output_shape=output_shape)

@property
Copy link
Member

Choose a reason for hiding this comment

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

I think the comments here are similar. The setter should allow Backend... and wrap it like in the constructor. Also I think it again needs to take care of CircuitSampler, that is setup in the constructor if the quantum instance is updated we need to redo that I think off the new quantum instance

qiskit_machine_learning/neural_networks/circuit_qnn.py Outdated Show resolved Hide resolved
@@ -112,6 +115,8 @@ def _compute_output_shape(self, interpret, output_shape, sampling) -> Tuple[int,
# this definition is required by mypy
output_shape_: Tuple[int, ...] = (-1,)
if sampling:
if not self._quantum_instance:
Copy link
Member

Choose a reason for hiding this comment

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

if self._quantum_instance is not None:

Copy link
Collaborator

@adekusar-drl adekusar-drl left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking care of this, really appreciated!

@stephenmurray2
Copy link
Contributor Author

@woodsp-ibm @adekusar-drl Thanks so much for the feedback, will address these comments shortly

Copy link
Collaborator

@adekusar-drl adekusar-drl left a comment

Choose a reason for hiding this comment

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

Please also make sure you reformat the code with black: make black. This is a new addition to the CI checks.

test/neural_networks/test_circuit_qnn.py Outdated Show resolved Hide resolved
qiskit_machine_learning/neural_networks/circuit_qnn.py Outdated Show resolved Hide resolved
@adekusar-drl
Copy link
Collaborator

Looks good to me, thanks.

stefan-woerner
stefan-woerner previously approved these changes Jun 2, 2021
Copy link
Contributor

@stefan-woerner stefan-woerner left a comment

Choose a reason for hiding this comment

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

lgtm

 into quantum-instance-checks

� Conflicts:
�	test/neural_networks/test_circuit_qnn.py
@adekusar-drl
Copy link
Collaborator

@stephenmurray2 Would you be able to finish the PR? There's just one open comment left.

@adekusar-drl
Copy link
Collaborator

@woodsp-ibm please take a look, the code is heavily revised, you may consider it as a brand new version.

adekusar-drl and others added 6 commits August 3, 2021 18:30
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>
@adekusar-drl
Copy link
Collaborator

@woodsp-ibm could please take a look?

qiskit_machine_learning/neural_networks/circuit_qnn.py Outdated Show resolved Hide resolved
qiskit_machine_learning/neural_networks/circuit_qnn.py Outdated Show resolved Hide resolved
Comment on lines 313 to 314
Returns:
None.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary because of the typehint. Please check other places.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed the return statements.

# this "if" statement is on purpose, to prevent sub-classes.
# pylint:disable=unidiomatic-typecheck
if type(op) == ListOp:
shapes = []
for op_ in op.oplist:
shape_ = self._get_output_shape_from_op(op_)
shape_ = self._compute_output_shape(op_)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
shape_ = self._compute_output_shape(op_)
shapes = [self._compute_output_shape(op_) for op_ in op.oplist]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated. Let's focus on the changes introduced in the PR.

Comment on lines +332 to +379
# sparse, sampling, quantum_instance_type, interpret (0=no, 1=1d, 2=2d), batch_size
(True, True, STATEVECTOR, 0, 1),
(True, True, STATEVECTOR, 0, 2),
(True, True, STATEVECTOR, 1, 1),
(True, True, STATEVECTOR, 1, 2),
(True, True, STATEVECTOR, 2, 1),
(True, True, STATEVECTOR, 2, 2),
(True, True, QASM, 0, 1),
(True, True, QASM, 0, 2),
(True, True, QASM, 1, 1),
(True, True, QASM, 1, 2),
(True, True, QASM, 2, 1),
(True, False, STATEVECTOR, 0, 1),
(True, False, STATEVECTOR, 0, 2),
(True, False, STATEVECTOR, 1, 1),
(True, False, STATEVECTOR, 1, 2),
(True, False, STATEVECTOR, 2, 1),
(True, False, STATEVECTOR, 2, 2),
(True, False, QASM, 0, 1),
(True, False, QASM, 0, 2),
(True, False, QASM, 1, 1),
(True, False, QASM, 1, 2),
(True, False, QASM, 2, 1),
(True, False, QASM, 2, 2),
(False, True, STATEVECTOR, 0, 1),
(False, True, STATEVECTOR, 0, 2),
(False, True, STATEVECTOR, 1, 1),
(False, True, STATEVECTOR, 1, 2),
(False, True, STATEVECTOR, 2, 1),
(False, True, STATEVECTOR, 2, 2),
(False, True, QASM, 0, 1),
(False, True, QASM, 0, 2),
(False, True, QASM, 1, 1),
(False, True, QASM, 1, 2),
(False, True, QASM, 2, 1),
(False, True, QASM, 2, 2),
(False, False, STATEVECTOR, 0, 1),
(False, False, STATEVECTOR, 0, 2),
(False, False, STATEVECTOR, 1, 1),
(False, False, STATEVECTOR, 1, 2),
(False, False, STATEVECTOR, 2, 1),
(False, False, STATEVECTOR, 2, 2),
(False, False, QASM, 0, 1),
(False, False, QASM, 0, 2),
(False, False, QASM, 1, 1),
(False, False, QASM, 1, 2),
(False, False, QASM, 2, 1),
(False, False, QASM, 2, 2),
Copy link
Contributor

Choose a reason for hiding this comment

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

So many lines of data could be extracted to a separate file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, but left as it is now. There should a consolidated change across all tests. And I'm not sure moving out such things to a separate file will solve the issue. Test parameters will be hidden, so I'm not convinced. Could be a dedicated function with a lot of nested loops better. But to be discussed, I think.

Copy link
Member

Choose a reason for hiding this comment

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

Since it looks like the whole data set there for the test is easily computed, just combinations of the parameters, it seems like some sort of simplification should be easily doable in the future at some point if desired.

@adekusar-drl adekusar-drl merged commit 0a49359 into qiskit-community:main Aug 11, 2021
@adekusar-drl
Copy link
Collaborator

@dlasecki thanks a lot!
@stephenmurray2 thanks for looking into it. I'm sorry for the "goodfirstissue" label, apparently it was misleading. This issue was not a very good first issue. Appreciated!

manoelmarques added a commit to manoelmarques/qiskit-machine-learning that referenced this pull request Aug 24, 2021
…skit-community#79)

* added getter and setter, raises exception if no qi

* set sampler in qi setter, added tests

* added logic to setters, fixed tests

* fix black

* calling setter with instantiation

* setters no longer accept optional qi

* undid changes to docstrings

* reset output shape within qi setter

* revised implementation

* fix spell

* fix mypy

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/opflow_qnn.py

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

* code review

* updated docstrings

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* code review

* docstrings

Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: dlasecki <dal@zurich.ibm.com>
manoelmarques added a commit that referenced this pull request Aug 24, 2021
* Update default batch_size in quantum_kernel (#150)

The max circuits per job for most/all backend devices is 900. Setting batch_size=900 fixes an inefficient jobs being created.

Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>

* This adds the callback feature for NN Classifier and Regressor (#112) (#151)

* Added a wrapper `get_objective` method in `TrainableModel`, and `callback` argument to NN classifier

`get_objective` returns a callable which is passed as objective function to NN classifier.

It checks for a callable `callback` argument, which if not `None` can access the intermediate data during the optimization.

* style changes

* Updated callback tests for `NeuralNetworkClassifier`

* Added callback for `NeuralNetworkRegressor`

* Added tests for callback

* Refactored `get_objective`

* Added release note

* Updated `callback`  argument docstring

* Shifted `callback` parameter to parent class `TrainableModel`

* Added assertions for number of weights in `TestNeuralNetworkClassifier` `TestNeuralNetworkRegressor`

* Rephrased release note and debugged tests for callback

Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>

* Added ability to automatically handle categorical labels (#62) (#143)

Added categorical label encoding

* Added documentation

* Update qiskit_machine_learning/algorithms/classifiers/neural_network_classifier.py

* Update releasenotes/notes/cateforical-output-049e682adc9d1e28.yaml

* Update releasenotes/notes/cateforical-output-049e682adc9d1e28.yaml

* Update releasenotes/notes/cateforical-output-049e682adc9d1e28.yaml

* Cleaned and organized code

* Update qiskit_machine_learning/algorithms/classifiers/neural_network_classifier.py

* Update qiskit_machine_learning/algorithms/classifiers/neural_network_classifier.py

* Update qiskit_machine_learning/algorithms/classifiers/neural_network_classifier.py

* Split categorical encoding into two functions for classifier's fit and score methods

* Set sparse=False in one-hot encoder

* removed unnecessary copy of arrays

* Renamed methods and fixed label reshape

* Added explanatory comments

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>

* Add hybrid qnn unit tests for TorchConnector (#156)

* Add hybrid qnn unit tests

* Fix spelling

* Add docstrings

* Retry

* Fix style

* Register weight name

* Update test

* Separate hybrid tests

* Fix spelling

* Fix lint

* Add comments

* Remove unused functions

* Add register weight param. reno

* Clarify procedure

* Remove unused import

Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>

* Fixed variable name in file 03_quantum_kernel.ipynb . (#179)

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

* Added getter/setter, exception handling for OpflowQNN, CircuitQNN  (#79)

* added getter and setter, raises exception if no qi

* set sampler in qi setter, added tests

* added logic to setters, fixed tests

* fix black

* calling setter with instantiation

* setters no longer accept optional qi

* undid changes to docstrings

* reset output shape within qi setter

* revised implementation

* fix spell

* fix mypy

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/opflow_qnn.py

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

* code review

* updated docstrings

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* code review

* docstrings

Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Global random seed (#178)

* Made TrainableModel use global seed (issue #158)

Made TrainableModel use RandomState object seeded by algorithm_globals.random_seed to choose an initial point for the optimizer outside of warm start.

* Global random seed for QSVC constructor (issue #158)

The value of algorithm_globals.random_seed is being used as random_state parameter for SVC constructor during construction of QSVC object.

* Added Qiskit into requirements.txt (issu #158)

Added Qiskit~=0.26.0 into requirements.txt, as qiskit.utils contain algorithm_globals object.

* Added Qiskit into requirements.txt & improved code formatting (issue #158)

Added Qiskit~=0.26.0 into requirements.txt, as qiskit.utils contain algorithm_globals object.

* Removed unnecessary explicit requirement of Qiskit package (issue #158).

* Simplified random number generation using global seed (issue #158).

* random_state is now being randomly initialized only if not passed as parameter (issue #158).

* Parameter 'random_state' now initialized by 'random_seed' in kwargs (issue #158).

* Update qiskit_machine_learning/algorithms/trainable_model.py

* Reverted incorrect changes to requirements.txt (issue #158).

Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Fix `RawFeatureVector` copy (#182)

* Fix num_qubits

* Remove _num_qubits

* Fix style

* Add tests for copy, bind_parameters

* Add Fix RawFeatVec reno

* Fix typing

* Fix test

* Add reset registers

* Fix num_qubits>0

* Fix mypy conflict

* Fix black

* Update if check

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

* Add if check

* Add comment

* Modify reno

* code review

* code review

* updated docstrings

* code review

Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>

* Prepare 0.2.1 release

Co-authored-by: Anna Phan <9410731+attp@users.noreply.github.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: Darsh Kaushik <70013230+darshkaushik@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Matt Wright <44040188+mattwright99@users.noreply.github.com>
Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Martin Beseda <martinbeseda@seznam.cz>
Co-authored-by: Stephen Murray <stephenandrewmurray@gmail.com>
Co-authored-by: dlasecki <dal@zurich.ibm.com>
Co-authored-by: Martin Beseda <martin.beseda@vsb.cz>
gentinettagian pushed a commit to gentinettagian/qiskit-machine-learning that referenced this pull request Dec 14, 2021
…skit-community#79)

* added getter and setter, raises exception if no qi

* set sampler in qi setter, added tests

* added logic to setters, fixed tests

* fix black

* calling setter with instantiation

* setters no longer accept optional qi

* undid changes to docstrings

* reset output shape within qi setter

* revised implementation

* fix spell

* fix mypy

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/opflow_qnn.py

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

* code review

* updated docstrings

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* Update qiskit_machine_learning/neural_networks/circuit_qnn.py

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

* code review

* docstrings

Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: dlasecki <dal@zurich.ibm.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.

Verify CircuitQNN and OpflowQNN work correctly when no QuantumInstance is passed.
7 participants