Skip to content

Rewrite QSVM examples with ExecutionSession#711

Merged
TaliCohn merged 7 commits intoClassiq:mainfrom
TomerGoldfriend:open_qsvm_to_main
Jan 13, 2025
Merged

Rewrite QSVM examples with ExecutionSession#711
TaliCohn merged 7 commits intoClassiq:mainfrom
TomerGoldfriend:open_qsvm_to_main

Conversation

@TomerGoldfriend
Copy link
Member

PR Description

This PR modifies three QSVM examples in the library. Instead of using the closed constructor we define feature maps with Qmod and use ExecutionSession for train, validate, and predict steps.

Some notes

  • Please make sure that you placed the files in an appropriate folder

  • And that the files have indicative names.

  • Please note that Classiq runs automatic code linting, which may minorly alter some files.

    • If you're familiar with pre-commit, you may run pre-commit install, and then at each commit, your files will be altered in a similar way

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #1.    ## Classiq imports

why 'Classiq imports'? there are other packages here as well


Reply via ReviewNB

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #5.    def my_bloch_feature_map(data: CArray[CReal], qba: QArray[QBit]):

I personally don't like the my_xxx naming

maybe give it a descriptive name that does not collide with the library names?


Reply via ReviewNB

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Actually, here I was trying to avoid future library names.

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #6.        repeat(ceiling(data.len / 2), lambda i: RX(data[2 * i] / 2, qba[i]))

data[2 * i] / 2 - what does it mean? different than what you do on the next line. If this is the algorithm, maybe explain a bit why you do that? because it is not trivial


Reply via ReviewNB

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, this is just to get the bloch sphere \cos(x[0])|0> + e^{ix[1]}sin(x[1])|1>. The main point is to have the same scaling for x[0] and x[1] in the angles (the mapping I did gives a factor of 1/4 to both, I think). I will explain that.

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #16.    QSVM_BLOCH_SHPERE = create_model(main, out_file="qsvm")

maybe QSVM_BLOCH_SHPERE-> QSVM_BLOCH_SHPERE_qmod?


Reply via ReviewNB

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #1.    write_qmod(QSVM_BLOCH_SHPERE, "qsvm")

you can remove it if you use the out_file


Reply via ReviewNB

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #3.        Generate execution parameters based on the mode (train or validate).

maybe validation should be replaced with 'prediction'? as somtimes you don't validate


Reply via ReviewNB

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #32.    def construct_kernel_matrix(matrix_size, res_batch, train=False):

It seems that you can extract the kernel matrix from the res_batch without the matrix_size or train parameters, no?

Also I would rename contruct to extract.I would call it construct if it includes the executions as well


Reply via ReviewNB

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how easy it is to extract the info from the res_batch, as it returns as a list, from which you need to infer whether you passed train or predict. In general, I could manage only with the train flag, I think.

Probably there are better ways to define the flow, I will try to figure out something.

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #56.                        res_batch[count].counts.get("0" * num_output_qubits, 0) / num_shots

why not use 'parsed_counts'?


Reply via ReviewNB

Copy link
Member Author

Choose a reason for hiding this comment

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

I will check if it is shorter to do with parsed_counts, not sure.

@@ -3,7 +3,9 @@
{
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #80.        - train_labels: List of labels corresponding to the training data.

what kind of labels? binary?


Reply via ReviewNB

@@ -64,14 +64,7 @@
"cell_type": "code",
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

Line #6.    def generate_connectivity_map(list_size, sublist_size, type):

can you make it with networkx instead? or itertools?

if not - I think type is a saved name, would rename it


Reply via ReviewNB

@@ -2,13 +2,30 @@
"cells": [
Copy link
Collaborator

@orsa-classiq orsa-classiq Jan 8, 2025

Choose a reason for hiding this comment

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

ST(H,t,...) = [ST[]]... - you used twice the term ST but once with 3 arguments and once with 4. maybe you need additional name?


Reply via ReviewNB

@TomerGoldfriend TomerGoldfriend force-pushed the open_qsvm_to_main branch 2 times, most recently from db6a582 to 2e0e180 Compare January 9, 2025 17:04
@TaliCohn TaliCohn merged commit 3428a98 into Classiq:main Jan 13, 2025
2 checks passed
@github-actions
Copy link

🔥 High five, @TomerGoldfriend! You've hit the incredible milestone of 5 merged PRs! 🖐️✨

Your dedication to classiq-library is outstanding. You're not just contributing code; you're shaping the future of quantum computing! 🌠
We'd love to hear your thoughts on the project. Any ideas for new features or improvements? 🤔

You're a superstar! 🌟

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.

3 participants