sandialabs / pyGSTi

A python implementation of Gate Set Tomography
http://www.pygsti.info
Apache License 2.0
132 stars 55 forks source link

Replace unnecessary calls to ``.flatten()`` with ``.ravel()``. #431

Open rileyjmurray opened 2 months ago

rileyjmurray commented 2 months ago

As of the latest commit on develop (between releases of pyGSTi 0.9.12 and 0.9.13)m there are 102 hits for .flatten() in the pyGSTi codebase.

The documentation for flatten says that this returns a flattened copy of the input array. In the vast majority of situations it's preferable to use .ravel() instead, which only returns a copy when necessary. I'll make a PR in the near future for these changes.

This is spiritually similar to #429 and #430, but different enough to warrant its own tracking issue.

coreyostrove commented 2 months ago

Sounds great!

pcwysoc commented 2 months ago

@rileyjmurray Could you explain a bit more when .ravel() can be used instead of .flatten()? I'm confused about when a copy is necessary.

rileyjmurray commented 2 months ago

@pcwysoc, as a rule of thumb, copies aren't necessary. Python (or rather, numpy) makes them all the time. The main time when a copy would be important is if you're explicitly using in-place operations or if you're overwriting portions of an array.

import numpy as np

a = np.ones((3,5))
b = a.ravel()
b = b + 2
print(a) # unchanged, because the change to b1 wasn't specifically in-place.

b = a.ravel()
b += 2
print(a) # changed, since "+=" is an in-place operation

a = np.ones((3,5))
b = a.ravel()
b[::2] = 0.0  # overwrite every other element of b with 0.0.
print(a) # changed, since b shares memory with a
rileyjmurray commented 1 month ago

I've started working on this.

Here are common patterns I've seen where .flatten() can be safely replaced with .ravel():

I'll edit this comment as I go.