Skip to content

Conversation

@tonybruguier
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Jan 13, 2021
@tonybruguier tonybruguier marked this pull request as ready for review January 14, 2021 20:02
@tonybruguier tonybruguier requested review from a team, cduck and vtomole as code owners January 14, 2021 20:02
@tonybruguier
Copy link
Contributor Author

Hi,
I tried to be careful, but the size of some tensors is quite big (up to 8-dimensions!) and so it's possible that I made a mistake somewhere. I tried to have good test coverage, but I'm more than happy to add more tests, more comments, more assert statements, etc...

Thanks a lot in advance and this is not particularly urgent.

@mpharrigan
Copy link
Collaborator

density simulator -> mps simulator?

For this, I think you should really look into quimb (and the cirq.contrib.quimb simulator) to get a handle on the indices. The nicety afforded by quimb (or tensor network libraries in general) is you get to provide names for the indices instead of relying on their relative ordering or np.einsum letters or tensor[:, :, 1, :, :]

@tonybruguier tonybruguier changed the title Add 2D computation for density simulator Add 2D computation for MPS simulator Jan 16, 2021
@tonybruguier
Copy link
Contributor Author

tonybruguier commented Jan 16, 2021

I followed your advice and used Quimb, and the code is much nicer indeed.

The unit tests do pass on my machine, but not on Github because it cannot import quimb. I looked around and it seems other files in Cirq can import it, so I am not sure what is wrong. Do you have a suggestion?

Thanks so much for teaching me about Quimb! I'm a bit concerned that Cirq is just a subset of Quimb :)

PTAL

@tonybruguier tonybruguier changed the title Add 2D computation for MPS simulator MPS simulator can handle any topology (still required to have 1- or 2-qubit gates) Jan 16, 2021
Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

Looking really good. Main outstanding thing is docstrings and answering some of my questions. I explicitly marked some of my other comments as "optional", since this PR is functional and readable at present and don't want to block merging it on those ideas

@tonybruguier
Copy link
Contributor Author

Thanks. PTAL.

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

sweet

@tonybruguier
Copy link
Contributor Author

Thanks for all the help. I think I've addressed all the comments, but happy to do more if required.

@mpharrigan mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jan 30, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jan 30, 2021
@CirqBot CirqBot merged commit c52e52f into quantumlib:master Jan 30, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jan 30, 2021
@tonybruguier tonybruguier deleted the mps2d branch February 3, 2021 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Makes googlebot stop complaining.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants