search

ngnrsaa / qflex

Flexible Quantum Circuit Simulator (qFlex) implements an efficient tensor network, CPU-based simulator of large quantum circuits.
Apache License 2.0
97 stars 24 forks source link

Pybind can use list of strings as inputs. #210

Closed s-mandra closed 4 years ago

alexandrupaler commented 4 years ago

Are strings now the preferred way of sending data to pybind? Three weeks ago I designed the interface around lists of strings, then temp files were added exclusively instead of strings, and this PR is moving once more back to strings exclusively. I am confused with regard to the stability of the interface. Please see #195 for a discussion about how to solve the bugs introduced by the temp files.

alexandrupaler commented 4 years ago

After discussion with Orion, temp file bugs may be solved by using this PR in conjunction with the date interface from #195.

s-mandra commented 4 years ago

In my opinion, I would completely remove tempfile: it was my mistake to not complete the interface with both files and strings before you worked on the cirq interface, but I believe tempfiles makes the whole cirq interface less stable (possible hacks through on-the-fly changes of files, rely on the local fs, ...). If you need help with the migration from tempfile, I can help you.

Also, I would keep for the base python interface both files and strings: in some context, you want to use the python interface without using cirq. Indeed, in our cluster, cirq is surprisingly and unbearable slow and I cannot use the cirq interface.

Hope this clarified my intentions and sorry again for the confusion.

alexandrupaler commented 4 years ago

No problem. The intention of the data interface is to have a single place where you open files and automatically close them. I saw that interface as a separation between all of the following : 1) python bindings, 2) file handles and 3) cirq interface classes. We can disable temp files automatically in the data interface, and enable it only when we need some kind of debugging, or even writing QFlex circuits into files to be used without the Cirq interface (in a potential later version of the interface?)

alexandrupaler commented 4 years ago

@s-mandra : do you know why Cirq is slow on your cluster? That would be interesting to solve, if possible.

s-mandra commented 4 years ago

@s-mandra : do you know why Cirq is slow on your cluster? That would be interesting to solve, if possible.

No idea .. it can takes 10mins to load it ..

95-martin-orion commented 4 years ago

@s-mandra : do you know why Cirq is slow on your cluster? That would be interesting to solve, if possible.

No idea .. it can takes 10mins to load it ..

Just to give context: installing Cirq on my local machine also took 10mins prior to #198, so this isn't exclusive to the cluster.

alexandrupaler commented 4 years ago

Do you mean to load by "import cirq" in the script, or to install Cirq from clean? If the later, then Cirq is indeed slow.

95-martin-orion commented 4 years ago

Do you mean to load by "import cirq" in the script, or to install Cirq from clean? If the later, then Cirq is indeed slow.

Specifically installing from clean (i.e. with docker-compose build, prior to #198)

s-mandra commented 4 years ago

@s-mandra : do you know why Cirq is slow on your cluster? That would be interesting to solve, if possible.

No idea .. it can takes 10mins to load it ..

Just to give context: installing Cirq on my local machine also took 10mins prior to #198, so this isn't exclusive to the cluster.

No, it takes 10mins to import the module :) (not to install it!)

s-mandra commented 4 years ago

@alexandrupaler, let me know your thoughts and thanks a lot for your time!

s-mandra commented 4 years ago

@s-mandra : apologies for reviewing so late. seems ok, but did not find the time to test everything. I trust Orion's opinion. I've been caught with a lot of bureaucracy. Please merge it and I will do my best to integrated this changes in the data interface - or even remove that interface at all. At this point, my main concern is #133

Thanks @alexandrupaler and don't worry, take your time! This PR doesn't require any immediate change to the cirq interface, so no need to rush that now.