Adding compiled QASM to the result #688

delapuente · 2018-07-26T13:34:59Z

Details and comments

Compiled QASM is part of the Qobj entity but it is required by the Result interface. This patch copies the QASM to the Result instance to make it available.

ajavadia · 2018-07-26T17:59:29Z

I think this is fine to temporarily make the get_ran_qasm() method work. But as I mentioned in the issue, the Compiled QASM should not remain part of the Qobj at all. This will happen when all devices and all simulators start executing pure JSON-format Qobj (and not QASM text anymore).

diego-plan9 · 2018-07-31T09:41:51Z

qiskit/_result.py

+            experiment_result['compiled_circuit_qasm'] = qasm
+
+
+def _find_experiment_result(result, name):


Can you reeevaluate if this function is really needed? Seems we could avoid an extra function if it is made a one-liner with next(x for x in result['result'] if blah) (not really blocking anyway).

I try to avoid this kind of one-liners since they are, usually, harder to read. I can move it inside copy_qasm_from_qobj_into_result as an auxiliary function or a lambda variable, though.

I also dislike lots of small functions, but your choice.

diego-plan9 · 2018-07-31T09:42:56Z

qiskit/_result.py

+        if experiment_result['name'] == name:
+            return experiment_result
+
+    return None


This one I'm a bit more concerned about: is there really a case where it could really return None? If that is the case, line 427 would fail at:

experiment_result['compiled_circuit_qasm'] = qasm

As far as I know, nope, it would expose an inconsistency between the Qobj and Result instances but the linter complained if I did not add a return value for this code-path. I can add a warning and avoid accessing it if None is returned. Does it make sense?

Yeah I think explicitly taking care of the None case would be good.

I'm going to keep this as a private function. With the one-liner, I can not add the warning and by moving it inside the copy_qasm_from_qobj_into_result function, makes the function hard to read.

ajavadia · 2018-08-02T03:36:11Z

qiskit/_result.py

@@ -414,3 +414,22 @@ def get_qubitpol_vs_xval(self, nqubits, xvals_dict=None):
                    circuit_name, z_dicts[qubit_ind])

        return qubitpol, xvals
+
+
+def copy_qasm_from_qobj_into_result(qobj, result):


shouldn't this be a private method?

I don't think so. It is related to the Result type but it does not seem like an operation on the Result type. Ideally, it should be placed near the simulators which are the ones using it but I did not want to add a new file nor I was sure about where to add it.

ajavadia · 2018-08-02T03:37:02Z

qiskit/_result.py

+
+
+def copy_qasm_from_qobj_into_result(qobj, result):
+    """Find the several QASM belonging to the Qobj experiments and copy them


grammar nit: "Find the QASMs belonging to ..."

ajavadia · 2018-08-02T03:43:12Z

qiskit/_result.py

+            experiment_result['compiled_circuit_qasm'] = qasm
+
+
+def _find_experiment_result(result, name):


I also dislike lots of small functions, but your choice.

ajavadia · 2018-08-02T03:45:41Z

qiskit/_result.py

+        if experiment_result['name'] == name:
+            return experiment_result
+
+    return None


Yeah I think explicitly taking care of the None case would be good.

ajavadia · 2018-08-02T03:50:50Z

test/python/test_wrapper_helpers.py

+        self.circuit.ccx(q[0], q[1], q[2])
+        self.circuit.measure(q, c)
+
+    def test_local_execute_and_get_ran_qasm(self):


Why create a new test file? Can you include in the the already existing test_wrapper.py?

I did it because test_wrapper.py seems to be testing the functions related to register() and adding this functionality was somehow polluting the test case.

Yeah but it should not just test register. As the name suggests, it should test the wrapper, which has several components (available_backends, register, execute).
Our test_wrapper.py needs to get more complete and this is a good addition to it.

ajavadia · 2018-08-02T03:52:36Z

@delapuente I see you modified the qasm_simulator files so they copy the qasm into result. Did you test that the devices and online simulators do the same thing?

delapuente · 2018-08-02T16:48:30Z

Unfortunately, I was trying to test if remote backends return the QASM but they took too long (more than 20 min.)

ajavadia · 2018-08-02T17:17:09Z

The online simulator should be quick.
The device, yes, could take longer. But let's make sure they are all consistent before merging.

delapuente · 2018-08-02T17:45:56Z

I can confirm get_ran_qasm() is working for the online simulator and devices. 🎉

ajavadia · 2018-08-04T07:16:14Z

Ok great. I added the qasm to the result generated by local_unitary_simulator too which seemed left out.

* Adding the regression test * Adding compiled QASM to the result * Disable CPP backends if not available * Avoid copying unexistent QASM code * Moving `get_ran_qasm` test into the `test_wrapper.py` test suite. * add qasm to the unitary simulator too

delapuente requested review from ajavadia, atilag, diego-plan9 and ewinston as code owners July 26, 2018 13:34

delapuente added this to the 0.6 milestone Jul 26, 2018

delapuente force-pushed the issue-671-get_ran_qasm branch 3 times, most recently from 47553d9 to 23a3c1a Compare July 30, 2018 09:57

diego-plan9 reviewed Jul 31, 2018

View reviewed changes

ajavadia reviewed Aug 2, 2018

View reviewed changes

delapuente force-pushed the issue-671-get_ran_qasm branch from 7b48eab to 52f500a Compare August 2, 2018 16:50

delapuente added 4 commits August 3, 2018 11:16

Adding the regression test

e7b7cf6

Adding compiled QASM to the result

ba82b87

Disable CPP backends if not available

a804c9b

Avoid copying unexistent QASM code

64d2125

delapuente force-pushed the issue-671-get_ran_qasm branch from 52f500a to 64d2125 Compare August 3, 2018 09:23

Moving get_ran_qasm test into the test_wrapper.py test suite.

94c3d70

delapuente force-pushed the issue-671-get_ran_qasm branch from 42b9e85 to 94c3d70 Compare August 3, 2018 09:56

diego-plan9 assigned diego-plan9 and ajavadia Aug 3, 2018

ajavadia and others added 2 commits August 4, 2018 02:34

add qasm to the unitary simulator too

2907bd8

Merge branch 'master' into issue-671-get_ran_qasm

31edca7

ajavadia approved these changes Aug 4, 2018

View reviewed changes

ajavadia merged commit cf06614 into Qiskit:master Aug 4, 2018

delapuente deleted the issue-671-get_ran_qasm branch November 8, 2018 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding compiled QASM to the result #688

Adding compiled QASM to the result #688

delapuente commented Jul 26, 2018 •

edited

Loading

ajavadia commented Jul 26, 2018

diego-plan9 Jul 31, 2018

delapuente Jul 31, 2018

ajavadia Aug 2, 2018

diego-plan9 Jul 31, 2018

delapuente Jul 31, 2018 •

edited

Loading

ajavadia Aug 2, 2018

delapuente Aug 2, 2018

ajavadia Aug 2, 2018

delapuente Aug 2, 2018

ajavadia Aug 2, 2018

ajavadia Aug 2, 2018

ajavadia Aug 2, 2018

ajavadia Aug 2, 2018

delapuente Aug 2, 2018

ajavadia Aug 2, 2018

ajavadia commented Aug 2, 2018 •

edited

Loading

delapuente commented Aug 2, 2018

ajavadia commented Aug 2, 2018

delapuente commented Aug 2, 2018

ajavadia commented Aug 4, 2018

		experiment_result['compiled_circuit_qasm'] = qasm


		def _find_experiment_result(result, name):



		def copy_qasm_from_qobj_into_result(qobj, result):
		"""Find the several QASM belonging to the Qobj experiments and copy them

Adding compiled QASM to the result #688

Adding compiled QASM to the result #688

Conversation

delapuente commented Jul 26, 2018 • edited Loading

Details and comments

ajavadia commented Jul 26, 2018

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

delapuente Jul 31, 2018 • edited Loading

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

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

ajavadia commented Aug 2, 2018 • edited Loading

delapuente commented Aug 2, 2018

ajavadia commented Aug 2, 2018

delapuente commented Aug 2, 2018

ajavadia commented Aug 4, 2018

delapuente commented Jul 26, 2018 •

edited

Loading

delapuente Jul 31, 2018 •

edited

Loading

ajavadia commented Aug 2, 2018 •

edited

Loading