Skip to content

Remove use of NumPy for scalar math in jellium.py#1358

Open
arettig wants to merge 1 commit into
quantumlib:mainfrom
arettig:jellium_math
Open

Remove use of NumPy for scalar math in jellium.py#1358
arettig wants to merge 1 commit into
quantumlib:mainfrom
arettig:jellium_math

Conversation

@arettig

@arettig arettig commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Previously, NumPy functions were used for scalar math in jellium.py. Using the Python standard math library instead should yield slight speedups.

Previously, NumPy functions were used for scalar math. Changing
to Python standard math library could yield to slight speedups.
@arettig arettig requested a review from mhucka June 11, 2026 17:22
@arettig arettig added the area/performance Involves code performance label Jun 11, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request replaces usages of the numpy library with the standard library math module for basic mathematical operations (such as pi, sqrt, cos, and floor) in jellium.py, allowing the removal of the numpy import. Additionally, a docstring has been converted to a raw string to properly handle LaTeX escape sequences. There are no review comments, and I have no further feedback to provide.

@mhucka mhucka left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good, with one possible issue. The math.cos function may given an error if momenta.dot() returns an array (even a 1-D array).

math.cos(momenta.dot(differences))

(Credit goes to Gemini for flagging this.) I didn't trace the code carefully to see if that can actually happen, but if there's a chance of that, maybe cast the dot product output explicitly to a scalar float before passing it to math.cos:

math.cos(float(momenta.dot(differences)))

Otherwise LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/performance Involves code performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants