-
Notifications
You must be signed in to change notification settings - Fork 13
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
Qubo simulations #34
base: qubo_simulations
Are you sure you want to change the base?
Qubo simulations #34
Conversation
@danlkv Sorry about the new PR. I'm copying your message from the other thread here.
|
Updates on the TODO items:
|
Updates/thoughts on the other problems you mentioned:
A cleaner implementation would require some modifications, but might ultimately be worth it. |
I think what's better is that the builder should be an argument of composer. For example: qtensor.DefaultQAOAComposer(G, gamma, beta, composer='qtree') The composer type could be a string identifier, or a builder object. By default we should use qtree. If it's a string identifier, use the |
@@ -7,14 +7,16 @@ | |||
'matplotlib' | |||
,'google-api-core[grpc]<=1.14.0' | |||
,'cirq' | |||
,'qiskit' | |||
,'qiskit[optimization]' |
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.
Why do you change this dependency?
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.
qiskit.optimization
, which used to be a part of qiskit aqua, is now deprecated and it's functionality has been moved into a package called qiskit-optimization
.
So, while from qiskit.optimization import QuadraticProgram
works for now, the recommended version is from qiskit_optimization import QuadraticProgram
. This is needed for one of the converter functions, namely graph_to_qiskitqubo
. Arguably though, that converter function can just be left out.
qiskit-optimization
is only an optional dependency of the qiskit
package. It can either be added either as qiskit[optimization]
, or separately as qiskit-optimization
.
https://github.com/Qiskit/qiskit-aqua/blob/main/README.md#migration-guide
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.
Let's use the current version, from qiskit_optimization import QuadraticProgram
then. Dependencies should have both qiskit
and qiskit-optimization
@@ -69,7 +69,7 @@ def test_qtree_energy(): | |||
G=G, gamma=gamma, beta=beta) | |||
|
|||
print('Energy', E) | |||
assert np.imag(E)<1e-6 | |||
assert np.abs(np.imag(E))<1e-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.
😅nice catch
|
||
return G | ||
|
||
def docplexqubo_to_graph(mdl): # mdl: DOcplex model |
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.
Hint: you can use python's built-in typing. It would look like this:
def docplexqubo_to_graph(mdl: Model)
Model
is imported in this file from docplex. Static code analyzers will be able to automatically catch uses where types don't match
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.
Thanks! I'll fix this today.
|
||
return _qubo_to_graph(n, qubo_constant, qubo_linear, qubo_quadratic) | ||
|
||
def qiskitqubo_to_graph(qp): # qp: qiskit_optimization quadratic program |
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.
from qiskit_optimization import QuadraticProblem
def qiskitqubo_to_graph(qp: QuadraticProblem):
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'll fix this too
|
||
for node in G.nodes(): | ||
if G.nodes[node]['weight'] is not None: | ||
total_E += self._get_node_energy(G, gamma, beta, node) |
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.
_get_node_energy
is undefined for this class. Just add a stub with a NonImplementedError
raised.
Maybe _get_edge_energy
should also raise and be implemented in QtreeSimulator, since currently it's defined on a superclass and would not work for CirqQAOASimulator
...
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.
Thanks a good point! I'll fix this.
trial_result = self.simulate(circuit) | ||
weight = G.nodes[node]['weight'] | ||
|
||
return weight*np.sum(trial_result.state_vector()) |
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.
Did you actually make cirq simulation to work? I remember there were some problems last time I tried
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 actually only tested the qtree
versions. I just wrote the cirq version to just be consistent with the QAOACirqSimulator
class, but didn't test it 😄. Should I just remove CirqIsingQAOASimulator
?
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 if we can't promise that this works, it shouldn't be there. But in case it works, it's fine.
print(f"Energy = {E}") | ||
assert np.abs(np.imag(E))<1e-6 | ||
|
||
for (n, p, d) in [(6, 2, 0), (6, 2, 3), (6, 2, 5)]: |
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.
For future note: there's such thing as test parametrization in pytest. Usage in qtensor: https://github.com/danlkv/QTensor/blob/dev/qtensor/tests/test_qaoa_energy.py#L32 and pytest docs
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.
Thanks. I'll change the tests to be inline with this
I agree that the builder would be better as a property of the composer, passed during initialization. Regarding the composers themselves, there may be a better way to organize them. It seems like the differences between Likewise, I'll see if I can put together a diagram (or a skeleton code) for these things |
There are 3 tasks here:
I think 1 should be completed first, then 2. If you are interested in doing so we can go for 3, and the practice from doing 2 will help. For diagrams, I recommend plantuml. There are several types of UML diagrams and the relevant for our case is Class diagram. For example, this is the picture that I had in mind for solving 3. |
I came across the concept of multiple dispatch recently. It's when different versions of a function can be defined for different types of arguments. It's kinda similar to function overloading in C++, except overloading is based on static types. It's more similar to Python's class-based oop, where depending on the type of the object, you can have different definitions for the same operation. Except, instead of doing this kind of dispatch based only on the first argument (self), in multiple dispatch you can do it for all arguments. I bring this up because, I think multiple dispatch can come in handy for refactoring QTensor. Apparently python has a few different packages which provide multiple dispatch support (eg: https://github.com/wesselb/plum). Maybe worth looking into? |
<Recreating an older PR. This time the source branch is directly based on the upstream branch (both
qubo_simulations
).>This PR extends QTensor to perform QAOA for general Ising Hamiltonians, which can contain constant, linear, and quadratic terms.
The Ising Hamiltonian is to be encoded as a networkx graph.
For a Hamiltonian H = \sum_{i ≠ j} (A_ij/2) s_i s_j + \sum_i B_i s_i + C
The nodes of the graph correspond to the variables s_i = -1 or 1.
A_ij is given by the 'weight' attribute of the edge between i and j. If an edge is absent, or doesn't have a 'weight', the corresponding A_ij is assumed to be 0.
B_i is given by the 'weight' attribute of the node i. If the 'weight' attribute is absent for a node, the corresponding B_i is assumed to be 0.
C is specified by a graph attribute called 'offset'.
Also included are
I've also added some package requirements:
scipy
,docplex
,qiskit[optimization]
. The first two are part of the dependency-tree of qtensor anyway.scipy
is used to validate the working of the qubo simulations for small problems, anddocplex
andqiskit_optimization
are used by the graph conversion utilities.