Pybind #55

alexandrupaler · 2019-09-20T22:12:01Z

Made it work with pybind11. This PR is to show that pybind11 worked almost out of the box.

pybind comes in the form of headers (these were copied in the include folder)
made some edits in Makefile (pybind compilation is make pybind)
the pybind_main.cpp includes a simple simulate method that is exposed to Python
python3 pybind_test to test the simulator with the default parameters like the example from main.cpp

Requirements for pybind11 are python3-dev (e.g. sudo apt-get install python3-dev)

95-martin-orion · 2019-09-20T22:41:08Z

I would recommend using a git submodule for pybind, rather than copying the code directly. Other than that, this looks good - the fact that it lets us keep C++17 behavior is enough reason to use it instead of Cython.

alexandrupaler · 2019-09-20T23:05:03Z

It is almost luck that pybind11 accepts some c++17 like std::optional. Otherwise I would have implemented it.

I added pybind11 as submodule.

95-martin-orion

We should decide on a directory structure for this at some point (e.g. C++ files in qflex/src, Python files in qflex/py) but that can be saved for a future PR.

pybind_main.cpp

95-martin-orion · 2019-09-23T15:40:17Z

pybind_test.py

@@ -0,0 +1,23 @@
+import qflex


Is there an equivalent of clang-format for Python that we can use to enforce Google style in this repo?

It looks like YAPF is the recommended solution here - let's handle this in a separate PR. (Opened #65 to track this - mark as resolved after reading.)

95-martin-orion

This PR has grown to cover much more than just pybind. Could you split off the commits into separate conceptual stages and send a PR for each? (At least one for 'pybind' and 'QflexInput'; others as you see the need for them.)

95-martin-orion

Sorry for taking so long on reviewing this - and thanks again for taking it on!

pybind_main.cpp

95-martin-orion · 2019-10-05T09:29:57Z

pybind_main.cpp

+  input.grid_width = grid_width;
+  input.super_cycles = super_cycles;
+
+  //deprecated?


Yes, this is deprecated and will be removed in #56. It can be removed here as well.

pybind_main.cpp

95-martin-orion · 2019-10-05T09:38:12Z

pybind_main.cpp

+  input.grid_data = &grid_stream;
+
+  /*
+      TODO: for the moment, the following two are not sent as parameters


Can this TODO be completed for this PR? It should only require passing initial_state and final_state_A as strings to simulate() (unless there's some python-to-C++ complexity I'm missing).

In place of the argc > current_arg checks, !{input_value}.empty() is fine.

95-martin-orion · 2019-10-05T09:50:54Z

pybind_main.cpp

+  for (int c = 0; c < amplitudes.size(); ++c) {
+    const auto &state = amplitudes[c].first;
+    const auto &amplitude = amplitudes[c].second;
+    std::cout << "XXXX"


It should be safe to use input.initial_state here - EvaluateCircuit() will default-initialize it if it wasn't provided as input.

95-martin-orion · 2019-10-05T09:58:25Z

pybind_test.py

@@ -0,0 +1,23 @@
+import qflex


It looks like YAPF is the recommended solution here - let's handle this in a separate PR. (Opened #65 to track this - mark as resolved after reading.)

95-martin-orion · 2019-10-05T10:10:37Z

pybind_test.py

@@ -0,0 +1,28 @@
+import qflex


File-level comment: Could you add a comment explaining the purpose of this file, and leave a TODO to convert it to a unit test (e.g. using pytest)?

s-mandra · 2019-10-06T19:47:13Z

If I may, I would suggest to avoid pybind and use callable C libraries to call from within python. It's much cleaner and callable libraries can be called from other languages like Julia.

alexandrupaler · 2019-10-06T20:05:34Z

pybind is headers only and super clean compared to cython. The result is a callable c++ lib (see Makefile).

Are you suggesting to use ctypes? I can look into that too, as the interface between cirq and qflex is a single function, thus lightweight.

However, I preferred at this stage pybind because it seemed faster to prototype, check and more robust against compilation errors.

s-mandra · 2019-10-07T01:35:13Z

pybind is headers only and super clean compared to cython. The result is a callable c++ lib (see Makefile).

Are you suggesting to use ctypes? I can look into that too, as the interface between cirq and qflex is a single function, thus lightweight.

However, I preferred at this stage pybind because it seemed faster to prototype, check and more robust against compilation errors.

I took a closer look to your code and I agree it's simpler than I thought (I never used pybind before), so I suggest to keep using pybind. Do you know if languages like Julia would be able to use the c++ lib?

alexandrupaler · 2019-10-08T21:08:00Z

I do not know if Julia is able to use the lib. It is an interesting question. I will look into that, after I made sure that the current Cirq integration is working.

s-mandra · 2019-10-10T19:45:01Z

Let me say that having qFlex working with Julia is not a priority for our first release. So I would just focus on Python. Thanks!

alexandrupaler added 3 commits September 20, 2019 21:13

repaired link issue

0ade49e

pybind example

90588d1

pybind example

4a757fe

submodule pybind

6616d76

forgot makefile

fbdb034

95-martin-orion requested changes Sep 23, 2019

View reviewed changes

95-martin-orion requested a review from benjaminvillalonga September 23, 2019 15:43

google format

f9897f3

95-martin-orion requested changes Oct 4, 2019

View reviewed changes

alexandrupaler force-pushed the pybind branch from a3c277e to f9897f3 Compare October 4, 2019 23:25

signature edits

98e50e9

95-martin-orion mentioned this pull request Oct 5, 2019

Enforce Python style guide with YAPF #65

Closed

95-martin-orion requested changes Oct 5, 2019

View reviewed changes

script cirq in venv

983e126

first trials with interface

e32437a

alexandrupaler added 3 commits October 8, 2019 23:19

add some doc to example

7885076

amp sim

df18474

uptodate with upstream

46c7d6b

s-mandra assigned alexandrupaler Oct 10, 2019

s-mandra added the enhancement New feature or request label Oct 10, 2019

s-mandra added this to the qFlex initial release milestone Oct 10, 2019

alexandrupaler added 2 commits October 10, 2019 23:15

added cirqinterface folder

f80378b

my makefile

4ace903

alexandrupaler added 7 commits October 11, 2019 00:17

my makefile

403bf40

amplitudes simulator impl

e48d8fa

add class for grids

aa50143

what did i do?

3d4814f

uptodate without Makefile

44197e9

rem pybind11 submodule

b21fea5

pybind11 compiles

9a74dc9

alexandrupaler closed this Oct 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pybind #55

Pybind #55

alexandrupaler commented Sep 20, 2019

95-martin-orion commented Sep 20, 2019

alexandrupaler commented Sep 20, 2019

95-martin-orion left a comment

95-martin-orion Sep 23, 2019

95-martin-orion Oct 5, 2019

95-martin-orion left a comment

95-martin-orion left a comment

95-martin-orion Oct 5, 2019

95-martin-orion Oct 5, 2019

95-martin-orion Oct 5, 2019

95-martin-orion Oct 5, 2019

95-martin-orion Oct 5, 2019

95-martin-orion Oct 5, 2019

s-mandra commented Oct 6, 2019

alexandrupaler commented Oct 6, 2019

s-mandra commented Oct 7, 2019

alexandrupaler commented Oct 8, 2019

s-mandra commented Oct 10, 2019

Pybind #55

Pybind #55

Conversation

alexandrupaler commented Sep 20, 2019

95-martin-orion commented Sep 20, 2019

alexandrupaler commented Sep 20, 2019

95-martin-orion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

95-martin-orion left a comment

Choose a reason for hiding this comment

95-martin-orion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-mandra commented Oct 6, 2019

alexandrupaler commented Oct 6, 2019

s-mandra commented Oct 7, 2019

alexandrupaler commented Oct 8, 2019

s-mandra commented Oct 10, 2019