XanaduAI / strawberryfields

Strawberry Fields is a full-stack Python library for designing, simulating, and optimizing continuous variable (CV) quantum optical circuits.
https://strawberryfields.ai
Apache License 2.0
754 stars 191 forks source link

Error when printing Result for stateless engine #356

Closed antalszava closed 4 years ago

antalszava commented 4 years ago

Issue description

Python version: 3.7.6 Platform info: Linux-4.19.11-041911-generic-x86_64-with-debian-buster-sid Installation path: /strawberryfields/strawberryfields Strawberry Fields version: 0.13.0.rc0 Numpy version: 1.18.1 Scipy version: 1.3.2 SymPy version: 1.5.1 NetworkX version: 2.3 The Walrus version: 0.12.0 Blackbird version: 0.2.3 TensorFlow version: 2.0.1

#### Source code and tracebacks

```python
import strawberryfields as sf 
from strawberryfields import ops 
from strawberryfields.utils import random_interferometer 

con = sf.api.Connection() 
eng = sf.RemoteEngine("X8", connection=con) 
prog = sf.Program(8) 
U = random_interferometer(4) 

with prog.context as q: 
    ops.S2gate(1.0) | (q[0], q[4]) 
    ops.S2gate(1.0) | (q[1], q[5]) 
    ops.S2gate(1.0) | (q[2], q[6]) 
    ops.S2gate(1.0) | (q[3], q[7]) 
    ops.Interferometer(U) | q[:4] 
    ops.Interferometer(U) | q[4:] 
    ops.MeasureFock() | q 

result = eng.run(prog, shots=1000)
print(result)

Additional information

The error simply comes from the fact that the state property raises an error in the stateless case. It is, however, also used in the definition for string representation in Result, therefore print(result) will indeed result in an error for stateless engines.

A possible solution could be creating a branching logic inside of __str__ without calling state in the stateless case.

co9olguy commented 4 years ago

A case for subclassing the result object? I feel like there may be other things that would also differentiate the "result" obtained via simulation versus that obtained remotely

antalszava commented 4 years ago

That's a great idea, as it would be an even cleaner solution! Could also help us if we expect that other factors would further differentiate the two types of Results.

josh146 commented 4 years ago

To be honest, it doesn't makes sense to print the state and the samples when simply print(results) is called.

Typically when you print an object in Python, you should just get high level metadata of the object, e.g.,

>>> print(results)
<Result: modes=4, backend=fock, origin=local, state_vector=True, shots=10> 

The state and the sample can then be printed separately using print(result.state) and print(result.sample).

Trying to print all data stored in the class at once, especially when some of the data can be exceptionally large, seems weird, and likely has a lot edge cases (e.g., unexpected bugs like this, the whole state might inadvertently show up in log files, the state could show up unexpectedly in Jupyter notebook environments, etc.)

A case for subclassing the result object?

I would argue no (for now), there isn't a clear logical separation between the two (yet), and I would rather we wait to see what that looks like before designing subclasses and abstract base classes.

Currently, local simulators can return either states and samples, just states, or just samples, depending on the program and the backend options, so this bug also affects the LocalEngine equally.

A possible solution could be creating a branching logic inside of str without calling state in the stateless case.

I would recommend just not printing states and samples when printing the top level results object.

pauljxtan commented 4 years ago

If I recall the lineage of this __str__ function correctly, this existed before the initial StarshipEngine refactor (during which the AttributeError was added for stateless results and this bug was introduced, so my bad :slightly_smiling_face:), and this issue was specifically brought up in that PR - but I don't exactly remember why it stuck around. I'm not sure if anyone really wants it at this point, and in hindsight I think it makes perfect sense to remove states and samples here.