Skip to content

⚡️ Improve Performance of getNqubits #959

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ystade
Copy link
Collaborator

@ystade ystade commented May 19, 2025

Description

This PR improves the performance of getNqubits by avoiding to constructing a set of qubits via getUsedQubits in most cases (except for compound operations).

This improvement was suggested here in PR 624 of QMAP.

Checklist:

  • The pull request only contains commits that are focused and relevant to this change.
  • I have added appropriate tests that cover the new/changed functionality.
  • I have updated the documentation to reflect these changes.
  • I have added entries to the changelog for any noteworthy additions, changes, fixes or removals.
  • I have added migration instructions to the upgrade guide (if needed).
  • The changes follow the project's style guidelines and introduce no new warnings.
  • The changes are fully tested and pass the CI checks.
  • I have reviewed my own code changes.

@ystade ystade self-assigned this May 19, 2025
@ystade ystade added the enhancement New feature or request label May 19, 2025
@ystade ystade added the Core Anything related to the Core library and IR label May 19, 2025
@ystade ystade removed their assignment May 19, 2025
@ystade
Copy link
Collaborator Author

ystade commented May 19, 2025

The CI will throw an error in this test.

This is actually a bug not related to this change and must be treated in the parsing. It should not be possible to construct a gate where control and target qubits overlap.

@ystade ystade added the fix Fix for something that isn't working label May 19, 2025
@burgholzer
Copy link
Member

This is actually a bug not related to this change and must be treated in the parsing. It should not be possible to construct a gate where control and target qubits overlap.

This is definitely a bug. Or at least a badly written test that has not been detected due to the lack of sanity checks. Would you mind submitting a PR that adds such checks and corrects the test?

Changes are probably necessary in the following places:

  • StandardOperation::StandardOperation(const Control control, const Qubit target,
    const OpType g,
    const std::vector<fp>& params)
    : StandardOperation(target, g, params) {
    controls.insert(control);
    }
    StandardOperation::StandardOperation(const Control control, const Targets& targ,
    const OpType g,
    const std::vector<fp>& params)
    : StandardOperation(targ, g, params) {
    controls.insert(control);
    }
    StandardOperation::StandardOperation(const Controls& c, const Qubit target,
    const OpType g,
    const std::vector<fp>& params)
    : StandardOperation(target, g, params) {
    controls = c;
    }
    StandardOperation::StandardOperation(const Controls& c, const Targets& targ,
    const OpType g,
    const std::vector<fp>& params)
    : StandardOperation(targ, g, params) {
    controls = c;
    }
  • SymbolicOperation::SymbolicOperation(const Control control, const Qubit target,
    const OpType g,
    const std::vector<SymbolOrNumber>& params)
    : SymbolicOperation(target, g, params) {
    controls.insert(control);
    }
    SymbolicOperation::SymbolicOperation(const Control control, const Targets& targ,
    const OpType g,
    const std::vector<SymbolOrNumber>& params)
    : SymbolicOperation(targ, g, params) {
    controls.insert(control);
    }
    SymbolicOperation::SymbolicOperation(const Controls& c, const Qubit target,
    const OpType g,
    const std::vector<SymbolOrNumber>& params)
    : SymbolicOperation(target, g, params) {
    controls = c;
    }
    SymbolicOperation::SymbolicOperation(const Controls& c, const Targets& targ,
    const OpType g,
    const std::vector<SymbolOrNumber>& params)
    : SymbolicOperation(targ, g, params) {
    controls = c;
    }

The addControl methods already check for duplicates, so it should not be an issue there.

Copy link

codecov bot commented May 26, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/ir/operations/CompoundOperation.cpp 0.0% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@burgholzer
Copy link
Member

@ystade I think this might need one additional test for the ci to be green

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Anything related to the Core library and IR enhancement New feature or request fix Fix for something that isn't working
Projects
Status: On Hold
Development

Successfully merging this pull request may close these issues.

2 participants