Skip to content

Add comments to qmod#1439

Merged
roie-d-classiq merged 11 commits intomainfrom
add_comments_to_qmod
Jan 20, 2026
Merged

Add comments to qmod#1439
roie-d-classiq merged 11 commits intomainfrom
add_comments_to_qmod

Conversation

@roie-d-classiq
Copy link
Collaborator

@roie-d-classiq roie-d-classiq commented Jan 18, 2026 •
edited
Loading

PR Description

Some notes

  • Please make sure that the notebook runs successfully with the latest Classiq version.

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

    • And that the file names are clear, descriptive, and match the notebook content.
      • Note that we require the file names of .ipynb and .qmod to be unique across this repository.
    • Plus, please make sure that all required files are included: .qmod, .synthesis_options.json, .metadata.json
    • And that images are embedded inside the notebook, not added as external files

  • If applicable, please include link to the paper on which the notebook is based, in the notebook itself.

  • Please use rebase on your branch (no merge commits)

  • Please link this PR to the relevant issue

  • Please make sure to run pre-commit when commiting changes

    • If you're using git in the terminal, make sure to install pre-commit via running pip install pre-commit followed by pre-commit install
    • Note that Classiq runs automatic code linting. Meaning that one of the tests verifies the output of pre-commit.
    • Also note that pre-commit may minorly alter some files. Make sure to git add the changes done by pre-commit

matanvax2
matanvax2 reviewed Jan 18, 2026
View reviewed changes
Copy link
Member

@matanvax2 matanvax2 left a comment

Choose a reason for hiding this comment

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

Looks nice!
I posted a few minor comments

algorithms/amplitude_amplification_and_estimation/quantum_counting/quantum_counting_iqae.qmod Outdated
// this qubit implements the marking oracle used inside amplitude amplification iterations.


// Defining the QStruct, containing the quantum variables a and b
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

This is Qmod/native, so the keyword is qstruct, not QStruct.
I use the following -

// Encapsulate the quantum variables a and b in a quantum struct

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

fixed

algorithms/amplitude_amplification_and_estimation/quantum_counting/quantum_counting_iqae.qmod Outdated
A_BITS: int = 2;
B_BITS: int = 2;

PROBLEM_QUBITS: int = A_BITS + B_BITS; // 4 qubits: a[2] || b[2]
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

This is redundant, the information is already in the definition of OracleVars.

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

right, fixed

algorithms/amplitude_amplification_and_estimation/quantum_counting/quantum_counting_iqae.qmod Outdated
allocate(problem_vars);
allocate(indicator);
est_var: qbit[];
problem_vars: qbit[PROBLEM_QUBITS];
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

Why declare it of type qubit array and not OracleVars? It makes more sense to use this abstraction, and it doesn't require a separate declaration of the sizes.

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

right!, fixed

algorithms/amplitude_amplification_and_estimation/quantum_counting/quantum_counting_qpe.qmod
@@ -1,23 +1,58 @@
// Quantum Counting via Quantum Phase Estimation (QPE)
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

Same comments from above apply here.

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

fixed

tutorials/basic_tutorials/add_bell_states/add_bell_states.qmod
@@ -1,3 +1,7 @@
// Add Bell states
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

Capitalize states

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

done

tutorials/basic_tutorials/quantum_primitives/hadamard_test/hadamard_test.qmod Outdated
}
}

TARGET_SIZE: int = 4; // Size of the target quantum variable
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

I don't think this is worth factoring out as it carries no additional meaning. IMO it's enough to use the declaration -

psi: qbit[4];

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

done

tutorials/basic_tutorials/quantum_primitives/hadamard_test/hadamard_test.qmod Outdated
allocate(1, expectation_value);
allocate(4, psi);
psi: qbit[]; //
allocate(1, expectation_value); // Ancilla qubit used for the Hadamard test
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

The '1' is redundant here. It's a single qubit variable.

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

yes, fixed

tutorials/basic_tutorials/quantum_primitives/hadamard_test/hadamard_test.qmod Outdated
allocate(4, psi);
psi: qbit[]; //
allocate(1, expectation_value); // Ancilla qubit used for the Hadamard test
allocate(TARGET_SIZE, psi); // Initializes the target state
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

Here the size is redundant, assuming that it is used to declare the variable (there is no size polymorphism in this code)

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

fixed

tutorials/basic_tutorials/quantum_primitives/hadamard_test/hadamard_test.qmod Outdated


// Performs the Hadamard test utilizing the within-applied function implementing
// V^dagger U V, where V is the Hadamard gate and U is the controlled_qft function.
Copy link
Member

@matanvax2 matanvax2 Jan 18, 2026

Choose a reason for hiding this comment

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

I think it's conventional to talk schematically using the sequence U V U-dagger, is it on purpose that this is different?

Copy link
Collaborator Author

@roie-d-classiq roie-d-classiq Jan 19, 2026

Choose a reason for hiding this comment

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

I followed the notebook, that's where the difference comes. I exchanged the U and V but left the dagger on the left operator since that is what is written in the within_apply documentation. In addition changed the ^ to -, this difference comes from a latex convention.

@matanvax2 matanvax2 self-requested a review January 20, 2026 08:38
matanvax2
matanvax2 approved these changes Jan 20, 2026
View reviewed changes
Copy link
Member

@matanvax2 matanvax2 left a comment

Choose a reason for hiding this comment

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

LGTM

@roie-d-classiq roie-d-classiq merged commit 7065b43 into main Jan 20, 2026
6 checks passed
@roie-d-classiq roie-d-classiq deleted the add_comments_to_qmod branch January 20, 2026 08:53
@github-actions
Copy link

github-actions bot commented Jan 20, 2026

🔥 Incredible, @roie-d-classiq! You've merged your 17th PR! 🎯🎊

Your ongoing commitment to classiq-library is truly remarkable. You're a driving force in our community! 🚀
Your contributions are helping to shape the future of quantum computing! What exciting features or improvements do you envision next? 🔮

We are grateful for your dedication! 💫

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matanvax2 matanvax2 matanvax2 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@roie-d-classiq @matanvax2