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

ComputeUncompute circuit transpilation option. #195

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rihartma
Copy link

@rihartma rihartma commented Jul 31, 2024

Summary

Solves #194

Add a new (optional) parameter to a ComputeUncompute class - a pass_manager. This pass manager is used to transform the compute-uncompute fidelity circuit to satisfy the pass manager constraints.

Details and comments

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2024

CLA assistant check
All committers have signed the CLA.

@Cryoris
Copy link
Collaborator

Cryoris commented Aug 1, 2024

Edit: I didn't see the issue explaining the problem with inverse (thanks @ElePT!), in this case it would make sense to add the pass manager 👍🏻

Thanks for opening a PR! However, user-specific compilation is not part of the primitives, especially the V2 primitives that will be the standard. The expected workflow is to just run the pass manager before passing the circuit into ComputeUncompute. Does that work in your use-case as well?

Copy link
Collaborator

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

I am in favor of adding the pass manager input, as @rihartma mentions in #194, the use of .inverse() modifies the set of gates in the circuit and requires it to be re-transpiled. I think this would be the simplest solution for ISA-compatibility across the library.

However, I think it's also important to note that this makes it the user responsibility to design a pass manager that does the job (for example, basis translation in this case) but is as light-weight as possible not to hurt performance in iterative algorithms. I don't think that should be a blocker though, if the alternative is the class not respecting the ISA constraint.

@woodsp-ibm what do you think?

@ElePT
Copy link
Collaborator

ElePT commented Aug 1, 2024

(also note that CI tests against main breaking is not an issue, it's unrelated to the content of the PR)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10186952664

Details

  • 3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.008%) to 90.421%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_algorithms/state_fidelities/compute_uncompute.py 3 4 75.0%
Totals Coverage Status
Change from base Build 10020859087: -0.008%
Covered Lines: 6362
Relevant Lines: 7036

💛 - Coveralls

@woodsp-ibm
Copy link
Member

A couple of observations first:

  1. As you may have noted in the readme here

    WARNING
    Qiskit Algorithms is no longer officially supported by IBM.
    Like any other Apache 2 licensed code, you are free to use it or/and extend it, but please be aware that it is under your own risk.

    and with this in mind the maintainers of Qiskit Machine Learning are copying over aspects of algorithms that are needed/used so as to be able to maintain them there and make ML independent of Qiskit Algorithms, and this includes the ComputeUncompute fidelity. See Migrating qiskit_algorithms qiskit-machine-learning#817

  2. V1 primitives, which is all algorithms support are going away shortly in the IBM Runtime (they have been deprecated now since earlier this year). This is really the only place where transpilation is needed in order to have things work

    https://docs.quantum.ibm.com/announcements/product-updates/2024-07-15-v1-support-ending

    Qiskit Runtime will no longer support V1 primitives after 15 August 2024.

    and as that is only a couple of weeks away the UncomputeCompute will not work at all then with Runtime as it would need updating to V2 primitives.

Given the above my take would be to just rely on ML going forwards for this functionality. If you need/want to use that in the next couple of weeks before V1 primitives are removed you can use a locally modified instance of the code.

Just a couple of comments on the PR anyway:

For me putting the Passmanager ahead of options and local, given that these can be passed positionally, may be a breaking change for some users that pass these. I know it fails on Runtime Sampler due to ISA but it still works with the reference Sampler from Qiskit and the Aer one. Such a change may break these latter cases.

It would need a test case for the new function and a release note.

But given the short time until this will no longer work with Runtime and with it being "moved" to ML, my take would be that its really too late now for this change. Thank you for the submission though.

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.

6 participants