-
Notifications
You must be signed in to change notification settings - Fork 1
Various sign/indexing fixes and performance improvements #2
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
Conversation
…atrix elements) up to flipping b field sign (complex conjugate and a sign).
- next to do the same for exchange matrix element
…change_pwme notebook
… charge carrier). - Added h5 file that has planewave and exchange elements for randomly sampled G points
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements sign and indexing fixes for the quantum Hall matrix elements package, removes the numerically unstable Gauss-Laguerre backend, and adds support for magnetic field sign conventions through a new sign_magneticfield parameter. The changes improve numerical accuracy and performance while fixing fundamental sign errors in the form factor and exchange kernel calculations.
Key changes:
- Removed the
gausslagbackend entirely due to numerical instability at large Landau level indices (n ≥ 12) - Fixed sign and indexing issues in form factors and exchange kernels by correcting the N-order calculation and parity factor formulas
- Added
sign_magneticfieldparameter to all public APIs to handle different magnetic field orientation conventions - Optimized the Gauss-Legendre backend with batched computations and symmetry exploitation
- Updated all tests to use the corrected conventions and added validation against analytical results
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| src/quantumhall_matrixelements/planewave.py | Added sign_magneticfield support; fixed angular phase calculation to use -sign_magneticfield factor |
| src/quantumhall_matrixelements/exchange_legendre.py | Major performance optimization with batched Bessel evaluations; fixed parity_factor formula from ((N+|N|)/2) to ((N-|N|)/2); increased default nquad to 8000 |
| src/quantumhall_matrixelements/exchange_hankel.py | Fixed N_order calculation from (n1-m1)-(m2-n2) to (n1-m1)+(m2-n2); fixed parity_factor; added extra_sgn term; added sign_magneticfield support |
| src/quantumhall_matrixelements/exchange_gausslag.py | Removed entire file (170 lines) due to numerical instability issues |
| src/quantumhall_matrixelements/diagnostic.py | Added get_form_factors_opposite_field and get_exchange_kernels_opposite_field helper functions; fixed transpose indices in symmetry check |
| src/quantumhall_matrixelements/init.py | Removed gausslag backend from dispatcher; imported new diagnostic helpers; updated error messages and documentation |
| src/quantumhall_matrixelements/_debug_symmetry.py | Removed debug utility file (50 lines) |
| tests/test_validation.py | Updated all tests to use gausslegendre instead of gausslag; added sign_magneticfield=-1 to all calls; relaxed tolerances from 1e-4 to 3e-3 |
| tests/test_form_factors.py | Added new test_sign_magneticfield_phase_relation to verify sign flip behavior |
| tests/test_exchange_legendre.py | Updated tests with sign_magneticfield parameter; removed reference to gausslag in comments |
| tests/test_exchange_kernels.py | Changed default method to gausslegendre; added test_exchange_kernel_sign_magneticfield_phase_relation |
| pyproject.toml | Added Sparsh Mishra as co-author |
| CITATION.cff | Added Sparsh Mishra as first author |
| README.md | Updated formulas to include σ notation; documented sign_magneticfield convention; removed gausslag backend from documentation; added wavefunction reference; updated citation |
| examples/plot_exchange_kernels_radial.py | Changed method from gausslag to gausslegendre in example |
| examples/compare_exchange_backends.py | Updated to compare gausslegendre vs hankel instead of gausslag vs hankel |
Comments suppressed due to low confidence (1)
src/quantumhall_matrixelements/init.py:85
- The functions
get_form_factors_opposite_fieldandget_exchange_kernels_opposite_fieldare imported from.diagnostic(line 18) but not included in__all__. If these are intended to be part of the public API, they should be added to__all__for better discoverability. If they're intended to be internal/advanced utilities, consider not importing them at the package level and document that users should import them fromquantumhall_matrixelements.diagnosticif needed.
from .diagnostic import get_form_factors_opposite_field, get_exchange_kernels_opposite_field
from .planewave import get_form_factors
from .exchange_hankel import get_exchange_kernels_hankel
from .exchange_legendre import get_exchange_kernels_GaussLegendre
if TYPE_CHECKING:
from numpy.typing import NDArray
ComplexArray = NDArray[np.complex128]
RealArray = NDArray[np.float64]
def get_exchange_kernels(
G_magnitudes: "RealArray",
G_angles: "RealArray",
nmax: int,
*,
method: str | None = None,
**kwargs,
) -> "ComplexArray":
"""Dispatcher for exchange kernels.
Parameters
----------
G_magnitudes, G_angles :
Arrays describing the reciprocal vectors :math:`G` in polar form.
Both must have the same shape; broadcasting is not applied.
nmax :
Number of Landau levels (0..nmax-1) to include.
method :
Backend selector:
- ``'gausslegendre'`` (default): Gauss-Legendre quadrature with rational mapping.
Recommended for all nmax.
- ``'hankel'``: Hankel-transform based implementation.
**kwargs :
Additional arguments passed to the backend (e.g. ``nquad``, ``scale``).
Common keywords include ``sign_magneticfield`` (±1) to select the
magnetic-field orientation convention.
Notes
-----
Both backends return kernels normalized for :math:`\\kappa = 1`. Any
physical interaction strength should be applied by the caller.
"""
chosen = (method or "gausslegendre").strip().lower()
if chosen in {"hankel", "hk"}:
return get_exchange_kernels_hankel(G_magnitudes, G_angles, nmax, **kwargs)
if chosen in {"gausslegendre", "gauss-legendre", "legendre", "leg"}:
return get_exchange_kernels_GaussLegendre(G_magnitudes, G_angles, nmax, **kwargs)
raise ValueError(f"Unknown exchange-kernel method: {method!r}. Use 'gausslegendre' or 'hankel'.")
try:
# Version is managed by setuptools_scm and exposed via package metadata.
__version__ = _metadata_version("quantumhall_matrixelements")
except PackageNotFoundError: # pragma: no cover - fallback for local, non-installed usage
__version__ = "0.0"
__all__ = [
"get_form_factors",
"get_exchange_kernels",
"get_exchange_kernels_hankel",
"get_exchange_kernels_GaussLegendre",
"__version__",
]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@skilledwolf I've opened a new pull request, #3, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@skilledwolf I've opened a new pull request, #4, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@skilledwolf I've opened a new pull request, #5, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: skilledwolf <18141588+skilledwolf@users.noreply.github.com>
…el.py Co-authored-by: skilledwolf <18141588+skilledwolf@users.noreply.github.com>
|
@skilledwolf I've opened a new pull request, #6, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: skilledwolf <18141588+skilledwolf@users.noreply.github.com>
Co-authored-by: skilledwolf <18141588+skilledwolf@users.noreply.github.com>
Co-authored-by: skilledwolf <18141588+skilledwolf@users.noreply.github.com>
Remove commented-out code and placeholder comments from exchange_hankel.py
Add sign_magneticfield validation to exchange_legendre.py
Add comprehensive docstring for get_exchange_kernels_GaussLegendre
Add missing blank line before Notes section header in README
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@skilledwolf I've opened a new pull request, #7, to work on those changes. Once the pull request is ready, I'll request review from you. |
…g, grammar, unused variable Co-authored-by: skilledwolf <18141588+skilledwolf@users.noreply.github.com>
Address PR review feedback: code style and validation fixes
Fixes sign and index ordering issues, adds validation against analytical results.