-
Notifications
You must be signed in to change notification settings - Fork 65
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
Defining the new Backend API classes #764
base: develop
Are you sure you want to change the base?
Conversation
interaction terms in the Hamiltonian. For an N-qudit system, | ||
must be an NxN symmetric matrix where entry (i, j) dictates | ||
the interaction coefficient between qudits i and j, ie it replaces | ||
the C/r_{ij}^6. |
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.
c_n/r{ij}^n? There is also XY.
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.
Pablo recently added a log_file
to our BackendConfig
it would cause the application to pipe all output into the log file. It seems like a thing we could generally support, though I'm also fine with keeping it an emu-mps specific config value.
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.
It sounds a bit EMU-MPS specific at this moment. I'd keep it there for now, we can always extend it to EmulationConfig
if it makes sense in the future
for j in range(state.n_qudits) | ||
] | ||
for i in range(state.n_qudits) | ||
] |
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.
We have recently had to add the following to this class
if hasattr(state, "get_correlation_matrix") and callable(
state.get_correlation_matrix
):
return state.get_correlation_matrix(
to resolve a performance bottleneck. We managed to improve the runtime of this callback a factor of 30 for the case we benchmarked. I would very much like to have some mechanism that prevents us from having to write an emu-mps specific callback to avoid a regression.
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.
I noticed that. I added a State.get_correlation_matrix()
method that raises NotImplementedError
by default but leaves it up to the State
subclass to define a custom way to calculate it with a controlled signature. I believe I'm covering what you are requesting in lines 226-229, no?
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.
I've done some thinking, and in the worst case, we can always override the Callback behaviour in the backend ourselves. This holds for all the comments I wrote in this file, so I don't consider any of it truly blocking, but some thought might deliver a more elegant solution.
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.
I think in the end it's best if you just take implementations that work well for state-vector, and we monkeypatch them in the emulators as needed.
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.
So, in your MPSConfig
or MPSConfig
, you would basically override apply()
of select observables at runtime?
) -> list: | ||
"""Calculates the observable to store in the Results.""" | ||
return [ | ||
CorrelationMatrix._get_number_operator( |
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.
Same as with the CorrelationMatrix, except it's less critical here. Maybe we can figure something out
) -> Any: | ||
"""Calculates the observable to store in the Results.""" | ||
# This works for state vectors and density matrices and avoids | ||
# squaring the hamiltonian |
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.
last week I found the following implementation to be significantly faster and more memory efficient for MPS. For state vector, the opposite is probably true. Another conundrum.
h_squared = H @ H
return h_squared.expect(state).real - H.expect(state).real ** 2
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.
Interesting, I thought that matrix-matrix multiplication would always be more resource intensive, so I tried to avoid it. Just goes to show how little I know about matrix product states, I guess
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.
This is the case that really convinced me that maybe emu-mps should just patch the contents of these methods at runtime.
On a separate note though, shouldn't you replace identity.expect(hstate)
with sqrt(h_state.overlap(h_state))
for performance?
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.
Yes, that's a good point
n_qudits=state.n_qudits, | ||
operations=[(1.0, [])], | ||
) | ||
return identity.expect(h_state) |
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.
same here with the overlap, I guess.
""" | ||
pass | ||
|
||
def get_correlation_matrix( |
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.
Based on my monkey patching idea, we could remove this. Having it is a bit of a slipperly slope, since why not also add a get_magnetization
and whatever other items you could think of.
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.
Sure, I'm fine with that
Results
class and makeResult
a subclass of it in backwards compatible wayState
andOperator
ABCsBackendConfig
and definesEmulationConfig
EmulatorBackend
ABCCallback
andObservable
ABCs