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

HamiltonianGate #4071

Merged
merged 43 commits into from
Apr 21, 2020
Merged

HamiltonianGate #4071

merged 43 commits into from
Apr 21, 2020

Conversation

dongreenberg
Copy link
Contributor

@dongreenberg dongreenberg commented Apr 3, 2020

Introducing HamiltonianGate, and tests. Tests pass. Also allows complex resolution of ParameterExpression.

Summary

HamiltonianGate takes a hermitian matrix or Operator and time parameter (I wanted to name this t but pylint didn't let me), and resolves to a Unitary gate equal to e^iHt. This allows such evolutions to be added to a circuit and the time parameter bound later.

Context: I believe this will be a major performance improvement in Aqua simulation, avoiding constructing large evolution circuits for ansatze like UCCSD.

@ajavadia
Copy link
Member

ajavadia commented Apr 3, 2020

I wanted to name this t but pylint didn't let me

You can edit .pylintrc and insert t as a good-name

@dongreenberg
Copy link
Contributor Author

@ajavadia should I?

Comment on lines +49 to +53
if hasattr(data, 'to_matrix'):
# If input is Gate subclass or some other class object that has
# a to_matrix method this will call that method.
data = data.to_matrix()
elif hasattr(data, 'to_operator'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be safer to check for types instead of attributes here? The to_matrix is probably safe both ways but to_operator might also be the attribute of another class than BaseOperator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to just replicate the API for UnitaryGate. Do you think it doesn't make sense here?

Copy link
Member

Choose a reason for hiding this comment

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

To operator lets you initialize with a quantum channel object in the case where that channel is really a unitary channel. This might be a not-needed edge case for this gate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually - I need something in here to be able to pass in Operators anyway, so is it a problem to leave it as is to handle the broader set of BaseOperators, or would it be better to change it to an isinstance(Operator)?

dongreenberg and others added 5 commits April 6, 2020 00:30
@dongreenberg dongreenberg requested a review from kdk April 20, 2020 03:16
kdk
kdk previously approved these changes Apr 20, 2020
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@kdk kdk added the Changelog: New Feature Include in the "Added" section of the changelog label Apr 20, 2020
@kdk kdk added the automerge label Apr 20, 2020
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, it needs a release note for the new feature but other than that there is the earlier nit that @1ucian0 pointed out in the tests around qobj. Not really a blocker, but would be good to cleanup since you need to iterate again

@dongreenberg
Copy link
Contributor Author

@mtreinish would you mind approving? It says I still need your approval as a codeowner for some reason (even though kdk already approved).

@jaygambetta
Copy link
Member

I am not a big fan of the name as it makes me think it is not simply a unitary with a Hermitian matrix as the input. This being said i have wasted 20 min thinking of a name and i cant get a better one so im good.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making the changes

@mergify mergify bot merged commit d0ea707 into Qiskit:master Apr 21, 2020
---
features:
- |
New `HamitlotianGate` and `QuantumCircuit.hamltonian()` methods are
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
New `HamitlotianGate` and `QuantumCircuit.hamltonian()` methods are
New `HamiltonianGate` and `QuantumCircuit.hamiltonian()` methods are

@dongreenberg
Copy link
Contributor Author

Thanks! Will correct the typo in the changelog

faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
* Introducing HamiltonianGate, and tests. Tests pass. Allow complex ParameterExpression.

* Fix some lint and bugs.

* Update qiskit/circuit/parameterexpression.py

Co-Authored-By: Julien Gacon <gaconju@gmail.com>

* Update qiskit/extensions/hamiltonian_gate.py

Co-Authored-By: Julien Gacon <gaconju@gmail.com>

* Update test/python/circuit/test_hamiltonian_gate.py

Co-Authored-By: Julien Gacon <gaconju@gmail.com>

* Update for Julien's changes.

* Update for Kevin's changes.

* Update for Kevin's changes.

* Update test/python/circuit/test_hamiltonian_gate.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Update test/python/circuit/test_hamiltonian_gate.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Remove deprecation warning, update for Chris's comments. Tests pass.

* Chris's suggestions.

* drawer

* Make hamiltonian_gate throw an error if QASM is called.

* Turn Kevin's changes

* Update test/python/circuit/test_hamiltonian_gate.py

Co-Authored-By: Kevin Krsulich <kevin@krsulich.net>

* Changelog

* Remove unnecessary qobj tests per Luciano.

* Update for merge.

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Kevin Krsulich <kevin@krsulich.net>
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
Co-authored-by: Luciano Bello <luciano.bello@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants