Skip to content

Sign mistakes etc in the exchange matrix elements (fixed in Gauss-Legendre backend only for now) #1

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.

Sign up for GitHub

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

Closed
skilledwolf wants to merge 3 commits into from
Closed

Sign mistakes etc in the exchange matrix elements (fixed in Gauss-Legendre backend only for now) #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e056721
- made changes that agree with the Julia version (analytic Exchange m…
SparshMishra97 Nov 30, 2025
fade8bb
Did checks and compared Julia implementation results to revised packa…
SparshMishra97 Nov 30, 2025
464cf35
added additional comments
SparshMishra97 Nov 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
452 changes: 452 additions & 0 deletions Checks_exchange_pwme.ipynb
View file
Open in desktop

Large diffs are not rendered by default.

Binary file added Exchange_pwme_julia_code.h5
View file
Open in desktop
Binary file not shown.
4 changes: 2 additions & 2 deletions examples/compare_exchange_backends.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@


def main() -> None:
nmax = 2
q = np.linspace(0.2, 3.0, 60)
nmax = 10
q = np.linspace(0.2, 20.0, 60)
theta = np.zeros_like(q)

X_gl = get_exchange_kernels(q, theta, nmax, method="gausslag")
Expand Down
40 changes: 33 additions & 7 deletions src/quantumhall_matrixelements/exchange_legendre.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@


def _N_order(n1: int, m1: int, n2: int, m2: int) -> int:
return (n1 - m1) - (m2 - n2)
#return (n1 - m1) - (m2 - n2)# change here
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

Remove commented-out code. The old implementation should be tracked in version control history, not left as comments in the codebase.

Suggested change
#return (n1 - m1) - (m2 - n2)# change here

Copilot uses AI. Check for mistakes.
return ((n1 - m1) + (m2 - n2))
Comment on lines 18 to +20
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The changes to the mathematical formulas in this backend will likely cause the test test_legendre_vs_hankel_small_n in tests/test_exchange_legendre.py to fail, as it compares results between the Gauss-Legendre and Hankel backends. Since the PR title mentions fixes are "for now" only in the Gauss-Legendre backend, consider:

  1. Updating the Hankel backend with the same corrections, or
  2. Temporarily skipping/modifying the comparison test with a note explaining the discrepancy, or
  3. Adding a comment in the test file explaining the expected difference

The inconsistency between backends could lead to confusion for users who expect identical results.

Copilot uses AI. Check for mistakes.


def _parity_factor(N: int) -> int:
"""(-1)^((N+|N|)/2) → (-1)^N for N>=0, and 1 for N<0."""
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The docstring for _parity_factor is now incorrect. It states (-1)^((N+|N|)/2) → (-1)^N for N>=0, and 1 for N<0, but the implementation has been changed to (-1) ** ((N - abs(N)) // 2), which has the opposite behavior: returns 1 for N>=0 and (-1)^N for N<0. Update the docstring to match the new implementation.

Suggested change
"""(-1)^((N+|N|)/2) → (-1)^N for N>=0, and 1 for N<0."""
"""(-1)^((N - |N|)/2): returns 1 for N >= 0, and (-1)^N for N < 0."""

Copilot uses AI. Check for mistakes.
return (-1) ** ((N + abs(N)) // 2)
#return (-1) ** ((N + abs(N)) // 2) # change here
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

Remove commented-out code. The old implementation should be tracked in version control history, not left as comments in the codebase.

Suggested change
#return (-1) ** ((N + abs(N)) // 2) # change here

Copilot uses AI. Check for mistakes.
return (-1) ** ((N - abs(N)) // 2) # CHANGE HERE NEGATIVE B FIELD


@lru_cache(maxsize=None)
Expand Down Expand Up @@ -141,7 +143,7 @@ def get_exchange_kernels_GaussLegendre(
# Integrand factor: exp(-z) * z^alpha * L * L * J
# alpha = (d1 + d2 - 1) / 2
alpha = 0.5 * (d1 + d2 - 1)

L1 = sps.eval_genlaguerre(p, d1, z)
L2 = sps.eval_genlaguerre(q, d2, z)

Expand All @@ -162,7 +164,9 @@ def get_exchange_kernels_GaussLegendre(
radial = (J_abs * term) @ w

signN = _parity_factor(N)
phase_factor = (1j) ** (d1 - d2)

#phase_factor = (1j) ** (d1 - d2) ## changed here
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

Remove commented-out code. The old implementation should be tracked in version control history, not left as comments in the codebase.

Copilot uses AI. Check for mistakes.
phase_factor = (1j) ** (d1 + d2) #CHANGE HERE NEGATIVE B FIELD
pref = (kappa * C / np.sqrt(2.0)) * phase_factor

else:
Expand All @@ -183,13 +187,35 @@ def get_exchange_kernels_GaussLegendre(
radial = (J_abs * term) @ w

signN = _parity_factor(N)
phase_factor = (1j) ** (d1 - d2)
#phase_factor = (1j) ** (d1 - d2) ## changed here
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

Remove commented-out code. The old implementation should be tracked in version control history, not left as comments in the codebase.

Suggested change
#phase_factor = (1j) ** (d1 - d2) ## changed here

Copilot uses AI. Check for mistakes.
phase_factor = (1j) ** (d1 + d2) # CHANGED HERE NEGATIVE B FIELD
pref = C * phase_factor

phase = np.exp(-1j * N * G_angles)
Xs[:, n1, m1, n2, m2] = (pref * phase) * (signN * radial)
phase = np.exp(-1j * N * G_angles) #CHANGED HERE NEGATIVE B FIELD
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The comment "CHANGED HERE NEGATIVE B FIELD" on this line is misleading because this line was not actually changed in this PR (no "+" prefix in the diff). If the sign convention was already correct, remove the comment. If it needs to be changed to np.exp(1j * N * G_angles), make that change. Misleading comments can confuse future maintainers.

Suggested change
phase = np.exp(-1j * N * G_angles) #CHANGED HERE NEGATIVE B FIELD
phase = np.exp(-1j * N * G_angles)

Copilot uses AI. Check for mistakes.
#phase = np.exp(1j * N * G_angles)
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

Remove commented-out code. The old implementation should be tracked in version control history, not left as comments in the codebase.

Suggested change
#phase = np.exp(1j * N * G_angles)

Copilot uses AI. Check for mistakes.

extra_sgn = (-1)**(n2-m2) # CHANGED HERE NEGATIVE B FIELD
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

[nitpick] Remove trailing whitespace from the comment on this line.

Suggested change
extra_sgn = (-1)**(n2-m2) # CHANGED HERE NEGATIVE B FIELD
extra_sgn = (-1)**(n2-m2) # CHANGED HERE NEGATIVE B FIELD

Copilot uses AI. Check for mistakes.

Xs[:, n1, m1, n2, m2] = (pref * phase) * (signN * radial) * extra_sgn # CHANGED HERE NEGATIVE B FIELD

return Xs


__all__ = ["get_exchange_kernels_GaussLegendre"]

"""
Suggestion for changes:
Add a B field direction option. -ve one just requires complex conjugate and sign factor

X^+_{n1,m1,n2,m2}(G) = (X^-_{n1,m1,n2,m2}(G))^* (-1)**(i - j + l - k)

F^+_{n',n}(q) = (F^-_{n',n}(q))^* (-1)**(n' - n)

Allan's code = -ve magnetic field
This package = +ve magnetic field

Comment on lines +206 to +216
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

This multi-line string is not assigned to any variable and appears after __all__, making it ineffective as module documentation. Consider either:

  1. Moving this content into the module docstring at the top of the file
  2. Creating a proper function/class to handle B-field direction as suggested
  3. Adding it as a comment in the relevant function's docstring

The current placement means it won't be accessible via help() or documentation tools.

Copilot uses AI. Check for mistakes.
Could add the actual wavefunction, hamiltonian and creation and
annihilation operators in the Readme so that its clear what the convention is for
either magnetic fields

"""
6 changes: 5 additions & 1 deletion src/quantumhall_matrixelements/planewave.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ def _analytic_form_factor(
laguerre_poly = eval_genlaguerre(n_min, delta_n_abs, arg_z)

# Phase convention: F_{n',n}(q) ∝ i^{|Δn|} e^{i (n - n') θ}
angles = (n_col - n_row) * q_angles + (np.pi / 2) * delta_n_abs
angles = (n_col - n_row) * q_angles + (np.pi / 2) * delta_n_abs #here
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

Remove the inline comment "#here" as it doesn't provide meaningful context. If an explanation is needed, consider adding a proper explanatory comment above the line.

Suggested change
angles = (n_col - n_row) * q_angles + (np.pi / 2) * delta_n_abs #here
angles = (n_col - n_row) * q_angles + (np.pi / 2) * delta_n_abs

Copilot uses AI. Check for mistakes.



Comment on lines +44 to +46
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

Remove unnecessary blank lines. These extra empty lines (lines 45-46) don't improve readability and should be removed.

Suggested change

Copilot uses AI. Check for mistakes.
angular_phase = np.cos(angles) + 1j * np.sin(angles)

F = (
Expand All @@ -50,6 +53,7 @@ def _analytic_form_factor(
* laguerre_poly
* np.exp(-0.5 * arg_z)
)

return F if F.ndim > 0 else F[()]


Expand Down