Conversation
The on-the-fly Price-McEwen Wigner-d recursion renormalises each step by bigi = 1/|dl_entry| and tracks lbig = log|dl_entry|. When an intermediate recursion value dl_entry is exactly zero (a node), bigi becomes inf and lbig becomes -inf, so dl_iter = inf*0 = NaN and the running log-norm goes to -inf. These NaNs are silently dropped by the nansum that accumulates ftm/flm, discarding that mode's contribution and producing percent-level errors. Exact-zero nodes are essentially never hit by the generic theta samples of mw/mwss/dh/gl sampling, but HEALPix rings sit at rational cos(theta) values that land exactly on nodes for spin != 0, so spin-2 HEALPix transforms carried few-percent pointwise errors (spin-0 was unaffected and existing HEALPix tests only covered spin 0). Guard the renormalisation so that where dl_entry == 0 we use bigi = 1 and lbig = 0, leaving lrenorm unchanged and setting the normalised value to 0, which is exactly consistent with the recursion bookkeeping. Applied to all four recursion paths (numpy/jax inverse and forward). Adds a regression test comparing the recursive HEALPix spin-1/2 inverse against the independent Turok-recursion base transform. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
==========================================
+ Coverage 96.10% 96.11% +0.01%
==========================================
Files 34 34
Lines 3539 3551 +12
==========================================
+ Hits 3401 3413 +12
Misses 138 138 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Author
|
Can we merge this into main at some point, as our project now depends on a hacked s2fft to function properly? |
christianhbye
added a commit
to slosar/croissant
that referenced
this pull request
Aug 10, 2026
Add the regression test owed by the polarization docs: the forward spin transform is validated against analytically sampled spin-weighted spherical harmonics (external ground truth, exercising croissant's compute_alm call), and the inverse against s2fft's independent Turok-recursion base implementation. Neither is a round trip, so the partially cancelling forward/inverse errors cannot mask the defect. Verified two-sided: 8/8 pass at the pinned slosar/s2fft revision; 6/8 fail on stock s2fft 1.4.0 (both test families). Doubles as the acceptance test for adopting the upstream release once astro-informatics/s2fft#387 merges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
christianhbye
added a commit
to christianhbye/croissant
that referenced
this pull request
Aug 11, 2026
* feat: add dense spherical harmonic engine Add a cached native-JAX dense transform path for low-band-limit workloads. Support independent HEALPix lmax selection, finite iterative refinement, packed m>=0 storage, cache management, and automatic differentiation. Document the API and validate equivalence, JIT behavior, gradients, and low-L/high-nside operation. * feat: add full-Stokes pair response support * feat: support topocentric polarized skies * Document the polarization + dense merge in the changelog Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: fold the polarization plan into docs/polarization.md Move the durable parts of POLARIZATION_PLAN.md into the normative convention document: the pair-response design rationale (why Jones patterns would introduce Wigner-3j coupling and break the diagonal-in-m time kernel), the public data model and its metadata contract, the spin-weighted dense cache behavior, the s2fft pin rationale, and the scalar API compatibility guarantees. Drop the plan document itself: the phase list, test plan, acceptance criteria, and co-development scheduling are working notes, not repository documentation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: pin ruff version to make lint pass * docs: add note about speed testing for dense vs s2fft * test: add engine equivalence tests for gradients and truncated lmax * docs: clarify recommended way to pre-warm dense matrix computation * fix: remove no-op dtype argument from dense matrix API The `dtype` argument of `precompute_dense_matrix` never affected the built matrix: both builders choose complex128/complex64 from JAX's x64 setting, which is what `s2fft.forward` itself produces regardless of the input map dtype. Its only effects were a duplicate cache entry per requested dtype and, in the non-HEALPix builder, an under-precise one_hot basis when float32 was requested under x64. Resolve the matrix dtypes in a single `_dense_dtypes()` helper and stamp the resolved complex dtype into the cache key. float32 and float64 maps now share one cached matrix, while toggling `jax_enable_x64` still changes the key so a complex64 matrix is never reused under x64. Forcing the matrix to a requested dtype instead was rejected: it would make the dense engine's output dtype diverge from s2fft's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: drop stale dtype argument left by the main merge Merging main into feat/full-polarization combined main's five-parameter _dense_matrix_key (dtype removed in 1a25240, since the matrix precision follows JAX's x64 setting rather than the map dtype) with the polarization branch's caller, which still passed data.dtype. Every compute_alm(..., engine="dense") call without a precomputed matrix therefore raised TypeError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: drop Python 3.10 from the test matrix The branch raises requires-python to >=3.11 (the pinned s2fft fork needs it), so uv refuses to sync a 3.10 environment and the matrix job fails before tests run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: certify the pinned s2fft HEALPix spin-recursion fix Add the regression test owed by the polarization docs: the forward spin transform is validated against analytically sampled spin-weighted spherical harmonics (external ground truth, exercising croissant's compute_alm call), and the inverse against s2fft's independent Turok-recursion base implementation. Neither is a round trip, so the partially cancelling forward/inverse errors cannot mask the defect. Verified two-sided: 8/8 pass at the pinned slosar/s2fft revision; 6/8 fail on stock s2fft 1.4.0 (both test families). Doubles as the acceptance test for adopting the upstream release once astro-informatics/s2fft#387 merges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: let release-please own the version Restore 5.2.1 (the manifest-tracked version); the 6.0.0 bump will come from release-please via a breaking-change squash title when the PR merges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: fix typos and the HEALPix link in math.md Addresses Copilot review feedback on #124; the typos predate the PR but the touched file made them visible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: drop premature version-6 references The version number is release-please's decision at merge time; describe features in the present tense and reference 5.2.1 concretely as the last Python 3.10-compatible release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: pin the psi orientation and the IAU circular-polarization label Define the basis-rotation angle psi (right-handed about the outward radial) so the spin labels are unambiguous and explicitly tied to the McEwen & Wiaux convention s2fft implements, and derive the IEEE/IAU label for the normative Stokes-V fixture: (1, +i) with exp(+i w t) is right-hand circular, so V = RCP - LCP = (RR - LL)/2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: fix pre-existing typo in the README simulator overview Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: drop unused multiprocessing scaffolding from the benchmark freeze_support() only matters for frozen Windows executables that spawn processes; the benchmark spawns none. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: certify the dense analysis matrix on complex input The existing direct-comparison test uses real input, for which a conjugated analysis matrix would only conjugate the output; complex input distinguishes the matrix from its conjugate and pins the VJP row-extraction convention at 1e-12, with a 1j-scaled batch entry as a second conjugation canary on the apply path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: state the horizon default and beam_rot units for PairStokesBeam Both are load-bearing behaviors of the polarized beam that the conventions document did not record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: pin the unpolarized reduction to the scalar pipeline An (I, 0, 0, 0) sky through an I-only pair response now provably reproduces the scalar convolve visibilities end to end (phases and multiple frequencies included), rather than the equivalence being implied by separate sky-side and beam-side alm tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Arnur Nigmetov <anigmetov@lbl.gov> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Christian Hellum Bye <chbye@berkeley.edu>
christianhbye
added a commit
to lusee-night/luseepy
that referenced
this pull request
Aug 11, 2026
croissant-sim v5.3.0.dev0 (the full-Stokes polarization merge, christianhbye/croissant@d4736db) is not on PyPI: croissant pins s2fft to the slosar fork carrying the HEALPix spin-recursion fix (astro-informatics/s2fft#387), and PyPI rejects packages with direct-URL dependencies. Until that lands upstream, luseepy pins the croissant tag directly and, because luseepy imports s2fft itself, co-pins the identical s2fft revision per croissant's guidance in docs/polarization.md. Both pins dissolve when s2fft#387 is released and croissant 5.3.0 reaches PyPI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 11, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: this bug was uncovered and fixed by agentic AI tools, but it is a real non-trivial bug whose fix should be merged. We need sasfe_abs for avoit hitting nans for auto-diff. There is a test added that fails in the current main version, but passes here, so it would be good to keep it. Below is the machine generated description:
The on-the-fly Price-McEwen Wigner-d recursion renormalises each step by bigi = 1/|dl_entry| and tracks lbig = log|dl_entry|. When an intermediate recursion value dl_entry is exactly zero (a node), bigi becomes inf and lbig becomes -inf, so dl_iter = inf*0 = NaN and the running log-norm goes to -inf. These NaNs are silently dropped by the nansum that accumulates ftm/flm, discarding that mode's contribution and producing percent-level errors.
Exact-zero nodes are essentially never hit by the generic theta samples of mw/mwss/dh/gl sampling, but HEALPix rings sit at rational cos(theta) values that land exactly on nodes for spin != 0, so spin-2 HEALPix transforms carried few-percent pointwise errors (spin-0 was unaffected and existing HEALPix tests only covered spin 0).
Guard the renormalisation so that where dl_entry == 0 we use bigi = 1 and lbig = 0, leaving lrenorm unchanged and setting the normalised value to 0, which is exactly consistent with the recursion bookkeeping. Applied to all four recursion paths (numpy/jax inverse and forward).
Adds a regression test comparing the recursive HEALPix spin-1/2 inverse against the independent Turok-recursion base transform.