Sign mistakes etc in the exchange matrix elements (fixed in Gauss-Legendre backend only for now) #1
Conversation
…atrix elements) up to flipping b field sign (complex conjugate and a sign).
There was a problem hiding this comment.
Pull request overview
This PR corrects sign and phase convention errors in the exchange matrix element calculations for negative magnetic field conventions. The corrections are applied only to the Gauss-Legendre quadrature backend.
Key Changes:
- Modified
_N_orderand_parity_factorfunctions to invert their behavior for negative B-field convention - Changed phase factor calculation from
(1j)**(d1-d2)to(1j)**(d1+d2)in both Coulomb and callable potential cases - Added an extra sign factor
(-1)**(n2-m2)to the final matrix element calculation
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
src/quantumhall_matrixelements/exchange_legendre.py |
Core fixes: updated _N_order, _parity_factor, phase factors, and added extra sign correction; includes extensive inline comments and end-of-file documentation about B-field conventions |
src/quantumhall_matrixelements/planewave.py |
Minor: added inline comment and blank lines (no functional changes) |
examples/compare_exchange_backends.py |
Increased test parameters from nmax=2 to nmax=10 and q range from 3.0 to 20.0 for more comprehensive backend comparison |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # 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 |
There was a problem hiding this comment.
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.
| 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 |
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Remove unnecessary blank lines. These extra empty lines (lines 45-46) don't improve readability and should be removed.
| """ | ||
| 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 | ||
There was a problem hiding this comment.
This multi-line string is not assigned to any variable and appears after __all__, making it ineffective as module documentation. Consider either:
- Moving this content into the module docstring at the top of the file
- Creating a proper function/class to handle B-field direction as suggested
- 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.
| phase = np.exp(-1j * N * G_angles) #CHANGED HERE NEGATIVE B FIELD | ||
| #phase = np.exp(1j * N * G_angles) | ||
|
|
||
| extra_sgn = (-1)**(n2-m2) # CHANGED HERE NEGATIVE B FIELD |
There was a problem hiding this comment.
[nitpick] Remove trailing whitespace from the comment on this line.
| extra_sgn = (-1)**(n2-m2) # CHANGED HERE NEGATIVE B FIELD | |
| extra_sgn = (-1)**(n2-m2) # CHANGED HERE NEGATIVE B FIELD |
| def _N_order(n1: int, m1: int, n2: int, m2: int) -> int: | ||
| return (n1 - m1) - (m2 - n2) | ||
| #return (n1 - m1) - (m2 - n2)# change here | ||
| return ((n1 - m1) + (m2 - n2)) |
There was a problem hiding this comment.
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:
- Updating the Hankel backend with the same corrections, or
- Temporarily skipping/modifying the comparison test with a note explaining the discrepancy, or
- 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.
|
|
||
| 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 |
There was a problem hiding this comment.
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.
| phase = np.exp(-1j * N * G_angles) #CHANGED HERE NEGATIVE B FIELD | |
| phase = np.exp(-1j * N * G_angles) |
|
|
||
|
|
||
| def _parity_factor(N: int) -> int: | ||
| """(-1)^((N+|N|)/2) → (-1)^N for N>=0, and 1 for N<0.""" |
There was a problem hiding this comment.
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.
| """(-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.""" |
|
|
||
| def _N_order(n1: int, m1: int, n2: int, m2: int) -> int: | ||
| return (n1 - m1) - (m2 - n2) | ||
| #return (n1 - m1) - (m2 - n2)# change here |
There was a problem hiding this comment.
Remove commented-out code. The old implementation should be tracked in version control history, not left as comments in the codebase.
| #return (n1 - m1) - (m2 - n2)# change here |
| def _parity_factor(N: int) -> int: | ||
| """(-1)^((N+|N|)/2) → (-1)^N for N>=0, and 1 for N<0.""" | ||
| return (-1) ** ((N + abs(N)) // 2) | ||
| #return (-1) ** ((N + abs(N)) // 2) # change here |
There was a problem hiding this comment.
Remove commented-out code. The old implementation should be tracked in version control history, not left as comments in the codebase.
| #return (-1) ** ((N + abs(N)) // 2) # change here |
| 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 | ||
| #phase = np.exp(1j * N * G_angles) |
There was a problem hiding this comment.
Remove commented-out code. The old implementation should be tracked in version control history, not left as comments in the codebase.
| #phase = np.exp(1j * N * G_angles) |
2 participants
No description provided.