Skip to content

fixed wrong time value in Trotterized Unitary #8126#9388

Closed
BramDo wants to merge 5 commits into
Qiskit:mainfrom
BramDo:fixissue8126v5branch
Closed

fixed wrong time value in Trotterized Unitary #8126#9388
BramDo wants to merge 5 commits into
Qiskit:mainfrom
BramDo:fixissue8126v5branch

Conversation

@BramDo
Copy link
Copy Markdown

@BramDo BramDo commented Jan 18, 2023

Summary

fixes #8126

Details and comments

Fix wrong time value in Trotterized unitary #8126.
When Computing a suzuki decomposition of a Hamiltonian for a given time t, if t is a float, everything works fine.
If t is a Parameter object the value gets squared when it is bound.
This is tested with function test_suzuki_trotterization_time_parameter(self).

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jan 18, 2023
@qiskit-bot
Copy link
Copy Markdown
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@woodsp-ibm woodsp-ibm added Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. mod: opflow labels Jan 18, 2023
@javabster javabster requested review from kevinsung and removed request for manoelmarques June 12, 2023 13:15
ti = Parameter("t")
u2 = Suzuki(1, order=1).convert(ti * ham)
u2_t = u2.bind_parameters({ti: time})
self.assertEqual(u2_t, u1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In addition to asserting the operators are equal, add an assertion that explicitly ensures that the coefficient is 11 and not 121.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I will add the expliceit assertion.


if isinstance(operator.coeff, (float, ParameterExpression)):
coeff = operator.coeff
if not isinstance(coeff, float):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A positive condition is clearer than a negative condition:

if isinstance(coeff, ParameterExpression):

Copy link
Copy Markdown
Author

@BramDo BramDo Aug 21, 2023

Choose a reason for hiding this comment

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

I got a DeprecationWarning: The class qiskit.opflow.evolutions.trotterizations.suzuki.Suzuki is deprecatied as of qiskit-terra 0.24.0. I propose to add this condition in the Suzuki class in opflow, however this will only be a temporary aprovement because the opflow class is deprecated. In thw future PauliEvolutionGate could be used.

@jakelishman
Copy link
Copy Markdown
Member

With the removal of opflow in #11111, this PR is now obsolete. Thanks for the effort at the time, and sorry we weren't able to move it merge before opflow was removed.

@BramDo BramDo deleted the fixissue8126v5branch branch January 29, 2025 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. Community PR PRs from contributors that are not 'members' of the Qiskit repo

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Wrong time value in Trotterized unitary

5 participants