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 example #834

Merged
merged 73 commits into from
Jun 7, 2023
Merged

Qcnn example #834

merged 73 commits into from
Jun 7, 2023

Conversation

ihpcdingwj
Copy link
Contributor

Checklist:

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

@scarrazza
Copy link
Member

What is the difference between this and PR #695?

@jf-kong
Copy link
Contributor

jf-kong commented Mar 22, 2023

What is the difference between this and PR #695?

This PR is for an example (in the folder examples/qcnn_classifier) using the QCNN model based on PR #729. This is based on a previous suggestion by @MatteoRobbiati to have an extended example/tutorial to illustrate the use of the QCNN model. PR #695 is an outdated PR (prior to #729) that has since been closed.

@MatteoRobbiati MatteoRobbiati self-requested a review April 11, 2023 11:25
@MatteoRobbiati
Copy link
Contributor

Can you please merge the main branch on this, in order to avoid conflicts due to the already merged qcnn branch?

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (0a84cb2) 100.00% compared to head (a772fff) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #834   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           48        48           
  Lines         6595      6595           
=========================================
  Hits          6595      6595           
Flag Coverage Δ
unittests 100.00% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Thank you very much for this, it is very useful for a better understanding of the model you implemented.

Let me add some suggestions:

  1. this is not the first example in qibo based on an IPython implementation. The qPDF is another one. Personally, I think the notebook approach is very useful in terms of visualization of the problem (e.g. in the documentation), but I think this folder starts to be a bit messy.
    According to this, I purpose two solutions: the first one is to remove the implementation part from README.md (I mean from ## How to use the QCNN class), which is not needed since already shown into the notebook. On this way this example will follow the line of the qPDF one. The second is to change this example into a Python script as is done for all the examples in qibo and to move this notebook to the tutorials repository, in order to be used into the documentation.
    I prefer this second option, since notebooks are useful tools but very difficult to handle with several hands.
  2. please select only one of the two notebooks and fix it: there are some unused packages and modules there (e.g. gates, numpy etc.) and the qcnn package must be loaded properly (from qibo.models.qcnn import QuantumCNN);
  3. add your example here.

Some small suggestions follow.

# Quantum Convolutional Neural Network Classifier

Code at: [https://github.com/qiboteam/qibo/tree/master/examples/qcnn_classifier]
(https://github.com/qiboteam/qibo/tree/master/examples/qcnn_classifier).
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the space between [...] and (...) in order to make te link work.

examples/qcnn_classifier/README.md Outdated Show resolved Hide resolved
![RT](images/RT.PNG)

## How to use the QCNN class
For more details on the QuantumCNN class, please refer to the documentation. Here we recall some of the necessary arguments when instantiating a QuantumCNN object:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add link to the documentation when it is available (where you mention the docs).

@scarrazza
Copy link
Member

@ihpcdingwj is this ready for a second review pass?

@MatteoRobbiati
Copy link
Contributor

MatteoRobbiati commented May 8, 2023

What is the status here @ihpcdingwj?

Let me summarize some remaining tasks:

  • remove one of the two notebooks. You have two versions of the same example;
  • fix the imports in the notebook: some packages are unused;
  • fix the qcnn import by setting from qibo.models.qcnn import QuantumCNN;
  • please follow the general convention: first Standard library imports, then related third party imports, then
    local application/library specific imports.
  • add your example here;
  • since we decided to keep the jupyter notebook form, please remove the part of README.md which is at this point reduntant: the ##How to use the QCNN class is not needed if already described in the notebook.

@jf-kong jf-kong requested a review from MatteoRobbiati May 10, 2023 09:11
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.

I did some small fix to the notebook (like passing circuit.draw() to the print function, etc),
fixed the link2path in the README.md and changed the name of the notebook into qcnn_demo.ipynb.

The last suggestion is to explicitly write in the README.md that scikit-learn is needed to run your example. It should be better to link the official installation page.

I also suggest to rebase this branch on the master.

@scarrazza scarrazza added this to the Qibo 0.1.15 milestone May 17, 2023
@ihpcdingwj
Copy link
Contributor Author

ihpcdingwj commented May 29, 2023

I did some small fix to the notebook (like passing circuit.draw() to the print function, etc), fixed the link2path in the README.md and changed the name of the notebook into qcnn_demo.ipynb.

The last suggestion is to explicitly write in the README.md that scikit-learn is needed to run your example. It should be better to link the official installation page.

I also suggest to rebase this branch on the master.

Thanks @MatteoRobbiati for the editing. We have made the changes suggested. Could you also clarify how do we proceed with "rebase"?

@MatteoRobbiati
Copy link
Contributor

Thanks @MatteoRobbiati for the editing. We have made the changes suggested. Could you also clarify how do we proceed with "rebase"?

Thank you for the modification.
With rebase I mean this git operation, but I think in this case we don't have conflicts.

@jf-kong
Copy link
Contributor

jf-kong commented May 29, 2023

Thanks @MatteoRobbiati for the editing. We have made the changes suggested. Could you also clarify how do we proceed with "rebase"?

Thank you for the modification. With rebase I mean this git operation, but I think in this case we don't have conflicts.

Thanks @MatteoRobbiati. Have rewritten history :)

@MatteoRobbiati
Copy link
Contributor

Thanks @MatteoRobbiati. Have rewritten history :)

Perfect! I think we are ready to merge :)

@scarrazza scarrazza merged commit 6efff96 into master Jun 7, 2023
@scarrazza scarrazza deleted the qcnn-example branch June 14, 2023 12:44
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.

5 participants