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

Qcnn upgrade #1083

Merged
merged 51 commits into from
Feb 7, 2024
Merged

Qcnn upgrade #1083

merged 51 commits into from
Feb 7, 2024

Conversation

jykhoo1987
Copy link
Contributor

@jykhoo1987 jykhoo1987 commented Nov 9, 2023

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

3 'upgrades' implemented in qcnn class:

  1. arbitrary 2-qubit circuit can be used as input during initialization which will be used as the base circuit when building the convolution layer
  2. minimization is now implemented via qibo.optimizers.optimize, whereas it was only scipy.minimize previously
  3. introduced a flag to make copy of initial state during initialization. By default, it will check backend during class initialization and be turned on if backend is qibojit to get around the in-place update issue.

Credits to @rahula06 for implementing most of these changes.

We will work on the updated documentation and make a separate pull request for that.

@ihpcdingwj @jf-kong @rahula06

jykhoo1987 and others added 15 commits November 1, 2023 19:56
1. Now accepts arbitrary 2-qubit circuit blocks as input
2. Switched from scipy.optimize.minimize to qibo.optimizers.optimize
Update test for qcnn upgrade:
1. Now accepts arbitrary 2-qubit circuit blocks as input
2. Switched from scipy.optimize.minimize to qibo.optimizers.optimize
fix #print
fix pytest for 100% coverage of updated qcnn class
install tensorflow
remove backend settings in minimize
remove explicit sgd check since use case follows as per qibo.optimizer.optimize
make initial state copy in Predictions function if backend is qibojit
test qibojit backend case to create copy of initial state
add import qibo to check backend
add copy flag during initialization
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ea72695) 100.00% compared to head (40e39d1) 99.97%.
Report is 135 commits behind head on master.

Files Patch % Lines
src/qibo/models/qcnn.py 93.10% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            master    #1083      +/-   ##
===========================================
- Coverage   100.00%   99.97%   -0.03%     
===========================================
  Files           68       68              
  Lines         9790     9803      +13     
===========================================
+ Hits          9790     9801      +11     
- Misses           0        2       +2     
Flag Coverage Δ
unittests 99.97% <93.10%> (-0.03%) ⬇️

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.

Copy link
Contributor

@MatteoRobbiati MatteoRobbiati left a comment

Choose a reason for hiding this comment

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

Many thanks @jykhoo1987 for implementing this.
Some comments follow.

src/qibo/models/qcnn.py Outdated Show resolved Hide resolved
src/qibo/models/qcnn.py Outdated Show resolved Hide resolved
src/qibo/models/qcnn.py Outdated Show resolved Hide resolved
src/qibo/models/qcnn.py Outdated Show resolved Hide resolved
tests/test_models_qcnn.py Outdated Show resolved Hide resolved
@@ -303,3 +305,47 @@ def test_qcnn_training():
predictions.append(1)
labels = np.array([[1], [-1], [1]])
test_qcnn.Accuracy(labels, predictions)


def test_two_qubit_ansatz():
Copy link
Contributor

Choose a reason for hiding this comment

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

I would pass backend as argument of the tests here. With this you will execute the tests in all the backends. Now that the qibojit in-place update is fixed in the main of the qcnn model, all the tests should pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it suffices if we test the backends for the training, so we can keep it to the tests that involve training maybe

Copy link
Contributor

Choose a reason for hiding this comment

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

It always better to test the code for all the available backends, in order not to have surprises if an external user decides to use this model with, e.g. tensorflow and training it with some automatic differentiation method.

You only need to add backend as an argument of your tests, e.g.:

def test_two_qubit_ansatz(backend):
     # same code here

tests/test_models_qcnn.py Show resolved Hide resolved
src/qibo/models/qcnn.py Show resolved Hide resolved
rahula06 and others added 2 commits November 23, 2023 10:47
Co-authored-by: Matteo Robbiati <62071516+MatteoRobbiati@users.noreply.github.com>
Co-authored-by: Matteo Robbiati <62071516+MatteoRobbiati@users.noreply.github.com>
@scarrazza scarrazza added this to the Qibo 0.2.3 milestone Nov 29, 2023
@scarrazza
Copy link
Member

@jykhoo1987 I will wait for your change until later today, if you do not manage we postpone this feature for the next release.

@scarrazza scarrazza modified the milestones: Qibo 0.2.3, Qibo 0.2.4 Dec 1, 2023
@jykhoo1987
Copy link
Contributor Author

Apologies for the delay. @rahula06 will look into the rest of this PR later this week when he is back from leave.

rahula06 and others added 4 commits December 11, 2023 02:46
update in response to comments.
The last two training tests now test both qibojit and numpy backends.
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ea72695) 100.00% compared to head (894f6d9) 100.00%.
Report is 296 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1083   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           68        68           
  Lines         9790      9803   +13     
=========================================
+ Hits          9790      9803   +13     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

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.

Rahul Arvind added 2 commits February 6, 2024 17:10
Merge branch 'qcnn_upgrade' of https://github.com/qiboteam/qibo into qcnn_upgrade
@rahula06
Copy link
Contributor

rahula06 commented Feb 6, 2024

It seems that going back to the old test works, so that the qibojit backend test is indeed working. My best guess is that the codecov tool might not be able to evaluate these pytest fixtures very well. If you are okay, we could just keep the tests with the explicit parametrization to pass the checks. If anyone has better knowledge of how codecov works or could suggest an alternative, that would be great too :)

Copy link
Contributor

@MatteoRobbiati MatteoRobbiati 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 few comments follow.
Then I think we are ready :)

src/qibo/models/qcnn.py Outdated Show resolved Hide resolved
src/qibo/models/qcnn.py Outdated Show resolved Hide resolved
src/qibo/models/qcnn.py Outdated Show resolved Hide resolved
tests/test_models_qcnn.py Outdated Show resolved Hide resolved
rahula06 and others added 4 commits February 6, 2024 23:58
Co-authored-by: Matteo Robbiati <62071516+MatteoRobbiati@users.noreply.github.com>
Co-authored-by: Matteo Robbiati <62071516+MatteoRobbiati@users.noreply.github.com>
Co-authored-by: Matteo Robbiati <62071516+MatteoRobbiati@users.noreply.github.com>
Co-authored-by: Matteo Robbiati <62071516+MatteoRobbiati@users.noreply.github.com>
@rahula06
Copy link
Contributor

rahula06 commented Feb 6, 2024

Thanks, a few comments follow. Then I think we are ready :)

fixed, and still seems to pass all the tests!

Copy link
Contributor

@MatteoRobbiati MatteoRobbiati left a comment

Choose a reason for hiding this comment

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

Thanks.

@renatomello renatomello added the enhancement New feature or request label Feb 7, 2024
@MatteoRobbiati MatteoRobbiati added this pull request to the merge queue Feb 7, 2024
Merged via the queue into master with commit 3f367cc Feb 7, 2024
21 checks passed
@renatomello renatomello deleted the qcnn_upgrade branch February 7, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants