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

Fix Initialize.gates_to_uncompute method #12976

Merged
merged 9 commits into from
Aug 20, 2024

Conversation

ShellyGarion
Copy link
Member

@ShellyGarion ShellyGarion commented Aug 18, 2024

Summary

close #12969
Fix some code that has been removed in #12178, so that the Initialize.gates_to_uncompute method will not fail

Details and comments

Initialize.gates_to_uncompute is based on StatePreparation._gates_to_uncompute() that now contains the inverse of Isomtery in order to make it more efficient

@ShellyGarion ShellyGarion added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Aug 18, 2024
@ShellyGarion ShellyGarion added this to the 1.2.1 milestone Aug 18, 2024
@ShellyGarion ShellyGarion requested a review from a team as a code owner August 18, 2024 06:49
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

@coveralls
Copy link

coveralls commented Aug 18, 2024

Pull Request Test Coverage Report for Build 10465223853

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 89.576%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 4 91.73%
Totals Coverage Status
Change from base Build 10459812488: 0.03%
Covered Lines: 67591
Relevant Lines: 75457

💛 - Coveralls

@ShellyGarion
Copy link
Member Author

The CI failures should be fixed after #12979 will be merged

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

This is very related to the comment below: StatePreparation is a Gate but currently contains an Instruction (namely an Isometry). We should update this such that a gate contains only other gates, e.g. by doing something like

def _define_synthesis_isom(self):
        # ... previous code ...
        # we know Isometry produces only Gate objects in this case,
        # so we can directly compose it's definition
        isom = Isometry(self._params_arg, 0, 0)
        initialize_circuit.compose(isom.definition, inplace=True, copy=False)

@Cryoris
Copy link
Contributor

Cryoris commented Aug 20, 2024

Let's wait for @woodsp-ibm to confirm this is good, but this LGTM -- thanks for the updates! 🙂

Edit: I just saw #12969 (comment), so it's good to go 👍🏻

@Cryoris Cryoris added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Aug 20, 2024
@woodsp-ibm
Copy link
Member

@Cryoris I had checked it yesterday and it was all good #12969 (comment) but I just rechecked it now and used both initializer and state_preparation files from here and it passes the finance tests so its all good.

Copy link
Member

@woodsp-ibm woodsp-ibm left a comment

Choose a reason for hiding this comment

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

Passes the Finance tests now - thx!

@alexanderivrii alexanderivrii added this pull request to the merge queue Aug 20, 2024
@Cryoris
Copy link
Contributor

Cryoris commented Aug 20, 2024

Thanks for double checking @woodsp-ibm! (and thanks for remembering the merge queue @alexanderivrii 😉)

Merged via the queue into Qiskit:main with commit aa09a02 Aug 20, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Aug 20, 2024
* add back files needed in Initialize.gates_to_uncompute()

* add a test for Initialize.gates_to_uncompute() method

* fix a comment

* add release notes

* update gates_to_uncompute such that it will call Isometry

* remove unused imports

* transfer code from StatePreparation to Initialize.gates_to_uncompute

* update code following review

(cherry picked from commit aa09a02)
github-merge-queue bot pushed a commit that referenced this pull request Aug 20, 2024
* add back files needed in Initialize.gates_to_uncompute()

* add a test for Initialize.gates_to_uncompute() method

* fix a comment

* add release notes

* update gates_to_uncompute such that it will call Isometry

* remove unused imports

* transfer code from StatePreparation to Initialize.gates_to_uncompute

* update code following review

(cherry picked from commit aa09a02)

Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data preparation initializer.py is calling a method on StatePreparation that no longer exists
6 participants