Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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)

@ShellyGarion ShellyGarion requested a review from Cryoris August 20, 2024 04:32
@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>
# for free to join this conversation on GitHub. Already have an account? # 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