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

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

Closed
rileyjmurray opened this issue Apr 25, 2024 · 5 comments · Fixed by #451
Closed

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

rileyjmurray opened this issue Apr 25, 2024 · 5 comments · Fixed by #451
Labels
fixed-but-not-in-release-yet Bug has been fixed, but isn't in an official release yet (just exists on a development branch)
Milestone

Comments

@rileyjmurray
Copy link
Contributor

rileyjmurray commented Apr 25, 2024

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
Copy link
Contributor

Sounds great!

@pcwysoc
Copy link

pcwysoc commented Apr 29, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

rileyjmurray commented May 28, 2024

I've started working on this.

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

  • np.concatenate([ ... vec.flatten() ... ]), since the concatenation will allocate new memory anyway.
  • f(vec.flatten()) where f the return value of f is guaranteed not to point to its argument's memory. For example, f could be np.all, or np.sum, or np.isnan.
  • y = f(x).flatten() where f(x) returns a new array

I'll edit this comment as I go.

@sserita sserita added the fixed-but-not-in-release-yet Bug has been fixed, but isn't in an official release yet (just exists on a development branch) label Sep 19, 2024
@sserita sserita added this to the 0.9.13 milestone Sep 19, 2024
@sserita
Copy link
Contributor

sserita commented Sep 19, 2024

Fixed in #451

@sserita sserita closed this as completed Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed-but-not-in-release-yet Bug has been fixed, but isn't in an official release yet (just exists on a development branch)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants