Wires as Sequence with new indexing #983

dwierichs · 2021-01-05T14:35:16Z

Context
All details can be found in the previous PR attempt via subclassing Wires from orderedset.OrderedSet. #979
Here we keep the subclassing from collections.abc.Sequence but change the indexing behaviour to standard python
iterable behaviour like list or tuple.

…viour, incl. all tests

codecov · 2021-01-05T14:45:25Z

Codecov Report

Merging #983 (f9b52b0) into master (a2fa39c) will increase coverage by 1.10%.
The diff coverage is 98.63%.

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
+ Coverage   96.79%   97.89%   +1.10%     
==========================================
  Files         148      151       +3     
  Lines       10507    11059     +552     
==========================================
+ Hits        10170    10826     +656     
+ Misses        337      233     -104

Impacted Files	Coverage Δ
pennylane/qnodes/base.py	`98.67% <ø> (-0.66%)`	⬇️
pennylane/wires.py	`98.51% <97.56%> (-1.49%)`	⬇️
pennylane/_device.py	`96.11% <100.00%> (-3.38%)`	⬇️
pennylane/beta/devices/default_tensor.py	`95.23% <100.00%> (+0.08%)`	⬆️
pennylane/circuit_drawer/grid.py	`88.70% <100.00%> (-7.59%)`	⬇️
pennylane/circuit_graph.py	`98.59% <100.00%> (ø)`
pennylane/devices/default_gaussian.py	`98.94% <100.00%> (-0.35%)`	⬇️
pennylane/devices/default_mixed.py	`100.00% <100.00%> (ø)`
pennylane/devices/default_qubit_autograd.py	`98.21% <100.00%> (-1.79%)`	⬇️
pennylane/devices/default_qubit_tf.py	`91.04% <100.00%> (+1.21%)`	⬆️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2fa39c...f9b52b0. Read the comment docs.

…formance

…pdated accordingly

… instances than self

dwierichs · 2021-01-13T13:41:37Z

@mariaschuld Maybe you can give some input on this version of the Wires class?
The error log of the sphinx test is rather cryptic to me, it would be great if you could help me fix this.

josh146 · 2021-01-13T13:51:47Z

Hey @dwierichs, the reason for the error is the following:

WARNING: Tried to skip objects {'Sequence'} in module pennylane.operation, but they were not present.  Ignoring.

(I had to search the logs for WARNING, and there were quite a few benign ones 😆)

This is likely because one of the imports in operation.py changed.

It can be fixed by navigating to doc/code/qml_operation.py (where the documentation for the operation class is generated), and removing Sequence from the following:

.. automodapi:: pennylane.operation
    :no-heading:
    :include-all-objects:
    :skip: Sequence, Enum, IntEnum, Variable, ClassPropertyDescriptor, multi_dot, pauli_eigs, Wires

The skip statement essentially tells Sphinx "these objects are imported into this file, but are not defined here, so do not include them within the documentation for the pennylane.operation module".

mariaschuld · 2021-01-13T14:07:37Z

Thanks @dwierichs! Is the PR ready for review? Did you have a chance to see if the speed improvement still holds?

I don't mind reviewing even if the docs do not build yet error-free, and maybe I can help sorting out the issues and also do some profiling this side.

…richs/pennylane into Wires_as_Sequence_with_new_indexing

dwierichs · 2021-01-13T14:38:54Z

Yes, this PR is ready for review. I think there would be more things to do, but maybe it is best to single out this rather basic change, what do you think?
The speedup is not as nicely measurable now, because the method of Wires in which most time is spent is a different one between PL 0.13 and this PR, but the runtime behaviour for the small benchmark I used so far, on 10 qubits, is

version	overall runtime (20 executions on 10 qubits) in sec.
PL 0.13	4.88
PL 0.14.0.dev0 (current master)	4.4
this PR	3.18

I think this benchmark is somewhat imprecise, but given the properly different runtimes, I think one can claim a speedup :-)

dwierichs · 2021-01-14T10:54:05Z

The skip statement essentially tells Sphinx "these objects are imported into this file, but are not defined here, so do not include them within the documentation for the pennylane.operation module".

Great, thank you for the explanation and help! Is fixed.

mariaschuld

On it.

The speedup is not as nicely measurable now, because the method of Wires in which most time is spent is a different one between PL 0.13 and this PR, but the runtime behaviour for the small benchmark I used so far, on 10 qubits, is

Just to understand, do you mean there is less time spent in _process? Wouldn't that be good?

pennylane/wires.py

Co-authored-by: Maria Schuld <mariaschuld@gmail.com>

dwierichs · 2021-01-14T12:29:53Z

Just to understand, do you mean there is less time spent in _process? Wouldn't that be good?

Sorry, that was unclear. In the previous PR the cumtime spent in Wires.__init__ was a good measure for the partial cost induced by the class. Now the __init__ cost are way way smaller, because indexing does not call it any longer. This means that "the cost of the Wires class" is not dominated by the __init__ cumtime and therefore less well-defined. In short, I just wanted to point out, that this comparison is not very rigorous. :)

mariaschuld · 2021-01-14T12:59:11Z

Perfect!

I am still stunned by how few changes the new indexing requires, since indexing should change all sorts of things, like for w in wires statements etc... I need to play around a bit to understand this!

By the way, one could search for all occurrences of .labels[ and change the ones that refer to wires to [. Shall I make a PR into your branch doing that?

dwierichs · 2021-01-14T13:08:16Z

I fully agree, this change caused way less headaches than I anticipated!

Yes, one could replace indexing of .labels by indexing the Wires instance itself. However, this will call the labels eventually, so I guess it actually would be a tiny bit slower (but cleaner, so maybe it's better to do it).
Regarding the indexing with a slice there is a difference, if I see this correctly: For a Wires instance wires and a slice instance s, wires.labels[s] will return a tuple and wires[s] will return a Wires object.

dwierichs · 2021-01-15T11:47:26Z

The modifications of _process are WIP.

dwierichs · 2021-01-15T17:33:16Z

Current design choice:

an iterable passed to Wires.__init__ will be interpreted as flat, i.e. as long as the elements in the iterable are hashable, they become the labels in the Wires instance. The behaviour is thus consistent across all input types but one (see below), in particular including 0-d qml.numpy arrays, which are interpreted as single-element iterable, and Wires instances, of which the labels simply are inherited.
An exception of 1. is made for iterables containing Wires instances. Those are passed to Wires.all_wires , which is the recommended way of creating a Wires object from an iterable of Wires instances.
__contains__ does not test for containment sequentially any longer, i.e. [3,4] in Wires([1,2,3,4]) = False. This is consistent with 1. above, allowing for labels that are iterable themselves. Again, checking for containment of a Wires instance is the (only!) exception, resulting in Wires([3,4]) in Wires([1,2,3,4]) = True. Therefore, __contains__ is completely aligned with 1. and 2. and except for the Wires exception it behaves like "normal" python iterables like list or tuple (Note that [0,1] in [0,1,2] = False).

… performant. TF and Autograd DefaultQubit devices have very weird `wire_map`s, making test_compare_default_qubit fail currently.

…th_new_indexing Remove cases where lists of Wires are fed into Wires class

…for iterables that have hashable contents but raise an error when creating set from them

…it devices.

pennylane/devices/default_gaussian.py

chaserileyroberts · 2021-01-18T16:12:34Z

pennylane/devices/default_qubit_autograd.py

+
+    def map_wires(self, wires):
+        """Map the wire labels of wires using this device's wire map.
+
+        Args:
+            wires (Wires): wires whose labels we want to map to the device's internal labelling scheme
+
+        Returns:
+            Wires: wires with new labels
+        """
+        wires = Wires([w.item() if isinstance(w, tensor) else w for w in wires])
+        mapped_wires = super().map_wires(wires)
+
+        return mapped_wires


Since this is the same code with default_qubit_tf, would it make sense to move this code to DeviceQubit?

This would make all QubitDevices, including the default qubits and default mixed state qubit, check for tensor instances, which maybe is not necessary?

chaserileyroberts · 2021-01-18T16:14:34Z

pennylane/wires.py

+        except TypeError as e:
+            # Make sure it really was a hashability issue
+            if str(e).startswith("unhashable"):
+                raise WireError("Wires must be hashable; got {}.".format(wires)) from e


I think we should reraise e if it wasn't this error.

Actually, there was a dedicated error raised previously. But then codecov complained that that was not tested and I tried to find a test case raising this. Did not find one, maybe you can add it if you have an idea how to break this.

… consistent with __init__/_process

dwierichs · 2021-01-18T16:32:38Z

The latest design choice update removes the exception from _process for lists of Wires instances (point 2 of the list above) and also from __contains__.
This means that a Wires instance in an iterable will be treated like any other label and no merging/flattening is performed (For list-of-Wires -> Wires there is Wires.all_wires)
Due to this, the __contains__ method will not serialize the containment check when invoked with a second Wires instance:

Wires([2, 3]) in Wires(range(5)) # False

As it is useful to be able to check for all elements of a Wires instance to be contained in another Wires instance, we introduced contains_wires:

a = Wires(range(10))
b = Wires(range(3))
a.contains_wires(b) # True
b.contains_wires(a) # False

Note that contains_wires is currently very specific in the sense that it will only ever return True if the input is a Wires instance.

pennylane/wires.py

Co-authored-by: Chase Roberts <keeper6928@gmail.com>

chaserileyroberts

Looks good to me!

chaserileyroberts

Looks good to me!

chaserileyroberts · 2021-01-18T19:25:50Z

This PR had awesome benchmark improvements. Amazing work!


       before           after         ratio
     [a2fa39cd]       [1fa2bc44]
     <pull/988/merge~1>       <pull/1008/merge~2>
      8.40±0.06ms       6.02±0.2ms    ~0.72  asv.core_suite.CircuitEvaluation.time_circuit(10, 3)
-      15.8±0.2ms       11.1±0.4ms     0.70  asv.core_suite.CircuitEvaluation.time_circuit(10, 6)
-      22.5±0.2ms       15.7±0.6ms     0.70  asv.core_suite.CircuitEvaluation.time_circuit(10, 9)
      2.12±0.08ms      1.84±0.09ms    ~0.87  asv.core_suite.CircuitEvaluation.time_circuit(2, 3)
      3.16±0.04ms       2.68±0.1ms    ~0.85  asv.core_suite.CircuitEvaluation.time_circuit(2, 6)
       4.09±0.2ms       3.39±0.2ms    ~0.83  asv.core_suite.CircuitEvaluation.time_circuit(2, 9)
       4.02±0.1ms       3.12±0.1ms    ~0.78  asv.core_suite.CircuitEvaluation.time_circuit(5, 3)
-      6.80±0.1ms       4.90±0.3ms     0.72  asv.core_suite.CircuitEvaluation.time_circuit(5, 6)
      9.56±0.08ms       7.34±0.3ms    ~0.77  asv.core_suite.CircuitEvaluation.time_circuit(5, 9)
       10.0±0.6ms       7.86±0.4ms    ~0.78  asv.core_suite.GradientComputation.time_gradient_autograd(2, 3)
       28.8±0.3ms         21.3±1ms    ~0.74  asv.core_suite.GradientComputation.time_gradient_autograd(2, 6)
-      55.5±0.8ms         38.6±2ms     0.70  asv.core_suite.GradientComputation.time_gradient_autograd(5, 3)
-         195±1ms          139±2ms     0.71  asv.core_suite.GradientComputation.time_gradient_autograd(5, 6)
-      14.1±0.6ms       10.5±0.2ms     0.75  asv.core_suite.GradientComputation.time_gradient_tf(2, 3)
-      34.7±0.6ms       26.7±0.1ms     0.77  asv.core_suite.GradientComputation.time_gradient_tf(2, 6)
-        61.0±2ms       45.5±0.4ms     0.75  asv.core_suite.GradientComputation.time_gradient_tf(5, 3)
-         210±1ms          149±2ms     0.71  asv.core_suite.GradientComputation.time_gradient_tf(5, 6)
       10.0±0.1ms       7.86±0.3ms    ~0.78  asv.core_suite.GradientComputation.time_gradient_torch(2, 3)
-      28.2±0.2ms       21.0±0.1ms     0.74  asv.core_suite.GradientComputation.time_gradient_torch(2, 6)
-        57.7±2ms         38.2±1ms     0.66  asv.core_suite.GradientComputation.time_gradient_torch(5, 3)
-         204±5ms          138±3ms     0.68  asv.core_suite.GradientComputation.time_gradient_torch(5, 6)

dwierichs · 2021-01-18T20:04:42Z

Very happy to see that it systematically improved the performance! Thank you for reviewing and benchmarking!

dwierichs added 2 commits January 5, 2021 14:05

Change indexing behaviour of Wires class to standard python list beha…

b5fa2eb

…viour, incl. all tests

Modify _process

fcac93f

dwierichs changed the title ~~Wires as sequence with new indexing~~ Wires as Sequence with new indexing Jan 5, 2021

dwierichs added 8 commits January 5, 2021 17:21

resort to _labels within the Wires class, which actually improves per…

e2f309d

…formance

Added Marias approach to _process as a comment, to be discussed and u…

8be11fb

…pdated accordingly

black

f788634

remove illegal protected access instances on Wires._labels of other…

7152e10

… instances than self

Moved alternative version of wires.py::_process to (gitignored) tmp/

eb24202

black

20b11c4

pylint improvements, fix imports to collections.abc

062aca6

Merge branch 'master' into Wires_as_Sequence_with_new_indexing

5bea3ce

josh146 requested a review from mariaschuld January 13, 2021 13:52

Merge branch 'master' into Wires_as_Sequence_with_new_indexing

9b565e4

dwierichs added 2 commits January 13, 2021 15:21

removed Sequence from skip attribute in doc/code/qml_operation.rst

9c80fb5

Merge branch 'Wires_as_Sequence_with_new_indexing' of github.com:dwie…

88580d5

…richs/pennylane into Wires_as_Sequence_with_new_indexing

mariaschuld reviewed Jan 14, 2021

View reviewed changes

pennylane/wires.py Outdated Show resolved Hide resolved

Update pennylane/wires.py

beb6c5f

Co-authored-by: Maria Schuld <mariaschuld@gmail.com>

slimmed down _process leading to different rationale of Wires input

976f3ac

Changed _process to new design choice (see commentary), which is more…

168ade5

… performant. TF and Autograd DefaultQubit devices have very weird `wire_map`s, making test_compare_default_qubit fail currently.

chaserileyroberts mentioned this pull request Jan 15, 2021

Faster wire combining. #1005

Merged

mariaschuld and others added 7 commits January 18, 2021 15:03

remove cases where lists of Wires are produced

da2d972

black

9d83d77

fix codefactor issues

d522879

fix typo

6121dec

Merge pull request #1 from PennyLaneAI/dwierichs-Wires_as_Sequence_wi…

de2b84d

…th_new_indexing Remove cases where lists of Wires are fed into Wires class

added grid init error based on shape test. removed exception raising …

d254f66

…for iterables that have hashable contents but raise an error when creating set from them

Removed duplicated code in map_wires of TF and Autograd default qub…

bb17db7

…it devices.

chaserileyroberts reviewed Jan 18, 2021

View reviewed changes

introduced dedicated Wires.contains_wires method to make __contains__…

e85f213

… consistent with __init__/_process

chaserileyroberts reviewed Jan 18, 2021

View reviewed changes

pennylane/wires.py Outdated Show resolved Hide resolved

pennylane/wires.py Outdated Show resolved Hide resolved

dwierichs and others added 2 commits January 18, 2021 17:33

Update pennylane/wires.py

ae4677b

Co-authored-by: Chase Roberts <keeper6928@gmail.com>

Deleted commented out code

f9b52b0

chaserileyroberts reviewed Jan 18, 2021

View reviewed changes

chaserileyroberts approved these changes Jan 18, 2021

View reviewed changes

chaserileyroberts merged commit 1fa2bc4 into PennyLaneAI:master Jan 18, 2021

dwierichs deleted the Wires_as_Sequence_with_new_indexing branch January 18, 2021 20:06

mariaschuld mentioned this pull request Jan 20, 2021

Improve efficiency of Wires.map (and Wires overall) #975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wires as Sequence with new indexing #983

Wires as Sequence with new indexing #983

dwierichs commented Jan 5, 2021 •

edited

Loading

codecov bot commented Jan 5, 2021 •

edited

Loading

dwierichs commented Jan 13, 2021

josh146 commented Jan 13, 2021 •

edited

Loading

mariaschuld commented Jan 13, 2021

dwierichs commented Jan 13, 2021

dwierichs commented Jan 14, 2021

mariaschuld left a comment

dwierichs commented Jan 14, 2021

mariaschuld commented Jan 14, 2021

dwierichs commented Jan 14, 2021

dwierichs commented Jan 15, 2021

dwierichs commented Jan 15, 2021 •

edited

Loading

chaserileyroberts Jan 18, 2021

dwierichs Jan 18, 2021

chaserileyroberts Jan 18, 2021

dwierichs Jan 18, 2021

dwierichs commented Jan 18, 2021

chaserileyroberts left a comment

chaserileyroberts left a comment

chaserileyroberts commented Jan 18, 2021 •

edited

Loading

dwierichs commented Jan 18, 2021

Wires as Sequence with new indexing #983

Wires as Sequence with new indexing #983

Conversation

dwierichs commented Jan 5, 2021 • edited Loading

codecov bot commented Jan 5, 2021 • edited Loading

Codecov Report

dwierichs commented Jan 13, 2021

josh146 commented Jan 13, 2021 • edited Loading

mariaschuld commented Jan 13, 2021

dwierichs commented Jan 13, 2021

dwierichs commented Jan 14, 2021

mariaschuld left a comment

Choose a reason for hiding this comment

dwierichs commented Jan 14, 2021

mariaschuld commented Jan 14, 2021

dwierichs commented Jan 14, 2021

dwierichs commented Jan 15, 2021

dwierichs commented Jan 15, 2021 • edited Loading

chaserileyroberts Jan 18, 2021

Choose a reason for hiding this comment

dwierichs Jan 18, 2021

Choose a reason for hiding this comment

chaserileyroberts Jan 18, 2021

Choose a reason for hiding this comment

dwierichs Jan 18, 2021

Choose a reason for hiding this comment

dwierichs commented Jan 18, 2021

chaserileyroberts left a comment

Choose a reason for hiding this comment

chaserileyroberts left a comment

Choose a reason for hiding this comment

chaserileyroberts commented Jan 18, 2021 • edited Loading

dwierichs commented Jan 18, 2021

dwierichs commented Jan 5, 2021 •

edited

Loading

codecov bot commented Jan 5, 2021 •

edited

Loading

josh146 commented Jan 13, 2021 •

edited

Loading

dwierichs commented Jan 15, 2021 •

edited

Loading

chaserileyroberts commented Jan 18, 2021 •

edited

Loading