- 
                Notifications
    
You must be signed in to change notification settings  - Fork 70
 
Added decompose to the returned circuit of TrotterQRTE #252
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
Added decompose to the returned circuit of TrotterQRTE #252
Conversation
          Pull Request Test Coverage Report for Build 17728641718Details
 
 💛 - Coveralls | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix LGTM. If I have understood correctly, this would "unlock" full compatiblity with qiskit 2.2.0, but there would still be some failing tests with the 2.1.x series because of the issues discussed in #244 (comment). Would you say this is accurate? Or am I missing additional issues with 2.2.x that would prevent this full compatibility?
          
 If I'm not mistaken, you're correct on this! The failing tests do so because of bugs introduced in 2.1.0 and that will be fixed in 2.2.0. As far as I'm aware, there's no other problems with the 2.2.X series, so it should provide full compatibility.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, LGTM
| 
           It seems the coveralls report failed (though on coveralls.io all looks fine) -- maybe try pushing an empty commit to re-trigger the CI run? 🙂  | 
    
| 
           I think that our coveralls action might be out of date and that's why the check is failing. I will try to look into it within the next couple of days.  | 
    
| 
           I am going to merge this PR in the meantime, I have tested a couple of coveralls fixes but they didn't work, so I wouldn't really block progress on it.  | 
    
Summary
This PR fixes the
TrotterQRTEtests that are failing in the nightly CI.Details and comments
The nightly CI is currently failing because of the
TrotterVQEtests. This is due to Qiskit/qiskit#14934, which added ato_matrixmethod toPauliEvolutionGate.This PR has been done because there was a discrepancy between the documentation of
PauliEvolutionGateand its actual implementation. Namely, when callingStatevectoror the likes on a circuit using aPauliEvolutionGate, what would be returned would actually be the implementation of the evolution (using a Lie-Trotter formula for instance), whereasPauliEvolutionGatein itself represents the exact evolution.In our case, this meant that some tests were testing for the implementation, but were checked against the exact evolution when using the latest Qiskit version, which of course resulted in errors, especially since the tests only use a few time steps.
Similarly, when printing the
evolved_stateof aTimeEvolutionResult, thePaulIEvolutionGaterepresentation would appear, which may by misleading. Indeed, in the [5] cell of the associated tutorial, one can see that the label of the gate on the right of the circuit is the exact operator, while this is really its approximation.The fix for this is to
decomposetheevolved_statebefore returning it, or before using it to evaluate theaux_operators. If having a custom gate representing the time step approximation was desirable, we could define a custom gate defined using thedecomposed version of thePauliEvolutionGate.