Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define cirq.STATE_VECTOR_LIKE #2376

Merged
merged 5 commits into from
Nov 6, 2019
Merged

Define cirq.STATE_VECTOR_LIKE #2376

merged 5 commits into from
Nov 6, 2019

Conversation

Strilanc
Copy link
Contributor

  • Add list-of-qid-values to the supported STATE_VECTOR_LIKE types
  • Add tensor-of-amplitudes to the supported STATE_VECTOR_LIKE types
  • Add non-numpy sequences to supported STATE_VECTOR_LIKE types
  • Infer values-vs-tensor-vs-vector using shape, fail when ambiguous (can only occur for non-standard shapes involving qudits of dimension 1)
  • Make num_qubits argument of to_valid_state_vector optional (if qid_shape specified)

- Add list-of-qid-values to the supported STATE_VECTOR_LIKE types
- Add tensor-of-amplitudes to the supported STATE_VECTOR_LIKE types
- Add non-numpy sequences to supported STATE_VECTOR_LIKE types
- Infer values-vs-tensor-vs-vector using shape, fail when ambiguous (can only occur for non-standard shapes involving qudits of dimension 1)
- Make `num_qubits` argument of `to_valid_state_vector` optional (if `qid_shape` specified)
@Strilanc Strilanc requested review from dabacon and cduck October 18, 2019 22:11
@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Oct 18, 2019
# Conflicts:
#	cirq/sim/wave_function_simulator.py
# Full big-endian computational basis state index.
int,
# Per-qudit computational basis values.
Sequence[Union[int, bool]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since bools are ints, we can just say int instead of the union.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In [1]: isinstance(bool, int)                                                                                                   
Out[1]: False

Copy link
Collaborator

@cduck cduck Oct 18, 2019

Choose a reason for hiding this comment

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

>>> isinstance(True, int)
True
>>> issubclass(bool, int)
True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hahaha, I made a dumb mistake and @cduck showed me the error in my ways. isinstance(False, int) returns true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think that there's some user benefit to listing bool explicitly, but done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah these type annotations are for the user so there's no harm in being explicit

Copy link
Collaborator

@cduck cduck left a comment

Choose a reason for hiding this comment

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

I didn't review this thoroughly but I don't see any issues handling qudits.

cirq/sim/wave_function.py Outdated Show resolved Hide resolved
qid_shape: Tuple[int, ...],
dtype: Type[np.number], atol) -> np.ndarray:

if isinstance(state_like, int):
Copy link
Collaborator

Choose a reason for hiding this comment

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

See #2375.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I really do want it to be an integer and not a generic number such as a float. I suppose numpy ints would also be acceptable, though.

Comment on lines +397 to +406
if len(qid_shape) == prod:
if (not isinstance(state_like, np.ndarray) or
state_like.dtype.kind != 'c'):
raise ValueError(
'Because len(qid_shape) == product(qid_shape), it is '
'ambiguous whether or not the given `state_like` is a '
'state vector or a list of computational basis values for '
'the qudits. In this situation you are required to pass '
'in a state vector that is a numpy array with a complex '
'dtype.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is worrying. As a user, I would it expect it to work for all input sizes if it works for most input sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because 2**n > n for all n >= 0, you never see this unless you have qudits with dimension 1. I think this is a strange enough corner case that we don't have to ensure it works perfectly.

We could reduce the number of relevant cases by checking if the array has non-integer entries in it (in which case it must be a state vector) or if it has integers summing to more than one (in which case it must be a qudit value list). The only truly ambiguous case is something like shape=(2, 2, 1, 1), value=[1, 0, 0, 0] where the value is one-hot and has an ambiguous length.

Copy link
Collaborator

Choose a reason for hiding this comment

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

tfw your qudits have dimension 1

# Full big-endian computational basis state index.
int,
# Per-qudit computational basis values.
Sequence[Union[int, bool]],
Copy link
Collaborator

@cduck cduck Oct 18, 2019

Choose a reason for hiding this comment

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

>>> isinstance(True, int)
True
>>> issubclass(bool, int)
True

f'qid_shape={qid_shape!r}\n'
f'state={state!r}\n')

if state.dtype.kind[0] not in '?bBiu':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a better way to check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm honestly not sure. Multiply it by 0, add 0.1, and see if it's still 0?

Copy link
Collaborator

@cduck cduck Oct 18, 2019

Choose a reason for hiding this comment

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

>>> issubclass(np.int8, np.integer)
True
>>> issubclass(np.float64, np.integer)
False
# But
>>> issubclass(np.bool, np.integer)
False
>>> issubclass(np.bool, np.bool)
True

https://stackoverflow.com/questions/934616/how-do-i-find-out-if-a-numpy-array-contains-integers

Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking against numbers.Integral should verify that the variable obeys integer arithmetic.

>>> issubclass(int, numbers.Integral)
True
>>> issubclass(bool, numbers.Integral)
True
>>> issubclass(np.int8, numbers.Integral)
True
>>> issubclass(np.int16, numbers.Integral)
True
>>> issubclass(np.bool, numbers.Integral)
True
>>> issubclass(float, numbers.Integral)
False
>>> issubclass(np.float64, numbers.Integral)
False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seems to be something tricky going on. Although np.bool is integral, the dtype type of an array initialized to have dtype np.bool is not:

>>> issubclass(np.bool, numbers.Integral)                                                                                  
True
>>> issubclass(np.array([1, 2, 3], dtype=np.bool).dtype.type, numbers.Integral)                                            
False
>>> issubclass(np.array([1, 2, 3], dtype=np.int32).dtype.type, numbers.Integral)                                            
True

Copy link
Collaborator

@c-poole c-poole Oct 21, 2019

Choose a reason for hiding this comment

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

The trickery appears to be that when you ask for an array of np.bool you don't get an array of np.bool. Instead you get an array of np.bool_ which inherits from np.generic instead of np.int.

>>> test_array = np.array([1, 2, 3], dtype=np.bool)
>>> test_array.dtype.type == np.bool
False
>>> test_array.dtype.type
<class 'numpy.bool_'>
>>> test_array.dtype.type.__bases__
(<class 'numpy.generic'>,)
>>> np.bool.__bases__
(<class 'int'>,)
>>> test_array.dtype.type == np.bool_
True

Copy link
Collaborator

Choose a reason for hiding this comment

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

np.bool_s do not behave like numbers with respect to arithmetic since they do not inherit from np.number. I don't know of a way for a numpy array of boolean values to have a dtype other than np.bool_. Whenever you index such an array, you will get a np.bool_ object. In order to make a numpy array give you python bools it seems that you need to use the item function.

You will probably need to add a check for np.bool_ in addition to the numbers.Integral check.
if not (isinstance(state.dtype, np.bool_) or issubclass(state.dtype, numbers.Integral)):

If the array has at least one element, an equivalent check would be given by the below code since the item function would return a python bool which is a numbers.Integral.
if not isinstance(state.item(0), numbers.Integral):

As written, if someone passed in an empty array for state and an empty tuple for qid_shape, the output of this function will be a Scalar of type dtype. Is this desired?

_qudit_values_to_state_tensor(state=np.empty(shape=(0),dtype=np.int64),
                                                 qid_shape=tuple(), dtype=np.int64)
array(1)
_qudit_values_to_state_tensor(state=np.empty(shape=(0),dtype=np.int64),
                                                 qid_shape=tuple(), dtype=np.bool)
array(True)

@Strilanc Strilanc added the Ready for Re-Review For when reviewers take their time. label Oct 29, 2019
@Strilanc Strilanc requested a review from mpharrigan October 29, 2019 21:51
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.

looks comprehensive

@@ -1410,7 +1410,7 @@ def unitary(self,

def final_wavefunction(
self,
initial_state: Union[int, np.ndarray] = 0,
initial_state: 'cirq.STATE_VECTOR_LIKE' = 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean ... this really exacerbates the "wavefunction vs. state vector" terminology question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a point.

Comment on lines +397 to +406
if len(qid_shape) == prod:
if (not isinstance(state_like, np.ndarray) or
state_like.dtype.kind != 'c'):
raise ValueError(
'Because len(qid_shape) == product(qid_shape), it is '
'ambiguous whether or not the given `state_like` is a '
'state vector or a list of computational basis values for '
'the qudits. In this situation you are required to pass '
'in a state vector that is a numpy array with a complex '
'dtype.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

tfw your qudits have dimension 1

@Strilanc Strilanc added automerge Tells CirqBot to sync and merge this PR. (If it's running.) and removed Ready for Re-Review For when reviewers take their time. labels Nov 6, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 6, 2019
@CirqBot CirqBot merged commit 58a794f into master Nov 6, 2019
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 6, 2019
@CirqBot CirqBot deleted the initial_state branch November 6, 2019 01:10
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 6, 2019
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.

7 participants