Fix/normalization convention - #30
Merged
Merged
Conversation
Three more sites the refactor missed, all found by reviewing it. LoopVec and GPU scalar transforms still converted to the config's normalization. Both are documented as drop-in accelerators -- analysis_turbo says "Same API and output as SHTnsKit.analysis", and the GPU comment said "matching CPU analysis" -- so once the CPU path became orthonormal these disagreed with the very functions they mirror. The sphtor GPU paths still convert, correctly: their CPU twin analysis_sphtor still does. NOT runtime-verified: LoopVectorization is not installed in the test environment and there is no CUDA device here. The change follows from each function's stated contract, but both paths need exercising on real hardware. synthesis_qst_batch hardcoded Float64/ComplexF64 output; the same fix had already been applied to _synthesis_batch. Verified: a ComplexF32 batch now returns Float32 and matches the Float64 result to 1.8e-7. The QST _ml scale comprehension spanned l = im:ltr while analysis_sphtor_ml scales only l >= max(1,im), so the l=0 slot was scaled on one side and not the other. Harmless today because l=0 carries no vector part, but the two sides must agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both invariants the orthonormal refactor established were only checked by roundtrip tests, which close under EITHER convention and so could not detect a revert. Only mixing the two paths breaks, so the tests must compare paths. test/serial/test_plan.jl: the planned scalar transforms are now asserted EQUAL to the non-planned ones (exact ==, across :orthonormal/:schmidt/:fourpi), replacing a self-roundtrip whose own comment claimed the planned path "is not (yet) guaranteed to match the non-planned path" -- no longer true. test/parallel/test_mpi_audit_fixes.jl: dist_analysis is asserted equal to serial analysis coefficient-for-coefficient across the same three configs, and dist_synthesis is fed the serial analysis output directly. Serial suite 67268 -> 67278 assertions; MPI audit-fix suite green at 4 ranks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects that are wrong under either normalization convention.
fallback_standard was still a per-rank predicate in both plan constructors, even
though every sibling locality predicate was reduced. It selects which BRANCH
dist_analysis! takes, and the two branches enter different full-comm collectives:
on a pencil with more phi-partitions than columns the owner enters the planned
Allreduce while the empty ranks enter dist_analysis_standard's own. They never
pair and the job hangs. Now reduced in DistAnalysisPlan and DistSphtorPlan.
The distributed sphtor rrules never got the zero-cotangent handling their serial
twins received: a loss consuming only one of the two outputs hands the other a
ZeroTangent, and Matrix{ComplexF64}(::ZeroTangent) is a MethodError.
analysis_qst_batch hardcoded ComplexF64 spectral output, the same eltype defect
already fixed in _synthesis_batch and _synthesis_qst_batch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commits converted the distributed layer and QST but left the serial
sphtor pair, the packed evaluators, SHTPlan's vector methods, packed_cplx and the
GPU sphtor path still applying the config scale. That was worse than the original
split: `analysis_sphtor` returned DIFFERENT conventions depending on argument type
(ParallelDispatch routes PencilArrays to the distributed method and Matrices to
the serial one -- a factor of 3 apart at l=4,m=2 under :schmidt), and evaluating a
QST triple with SHqst_to_point was off by 1.93x in Vtheta and 6.09x in Vphi.
Now genuinely one convention. Conversions removed from:
src/sphtor_transforms.jl analysis/synthesis_sphtor, _l and _ml variants
src/local.jl SH_to_lat, SH_to_lat_cplx, SHqst_to_point,
SH_to_grad_point, SHqst_to_lat
src/plan.jl analysis_sphtor!/synthesis_sphtor!
src/complex_packed.jl synthesis/analysis_packed_cplx, synthesis_point_cplx
src/qst_transforms.jl the S/T round-trip added when only sphtor converted
src/batch_transforms.jl QST batch S/T handling
ext/SHTnsKitGPUExt.jl gpu_analysis_sphtor / gpu_synthesis_sphtor
ext/SHTnsKitAdvancedADExt _scale_cotangent in the sphtor rrules, and the M
factors in the packed_cplx adjoints
`convert_alm_norm!` itself stays: it is the supported way for callers who want
coefficients in a different convention.
Cross-API equality now holds at machine precision, identically for
:orthonormal, :schmidt and :fourpi/cs_phase=false -- the error figures no longer
depend on the norm at all:
QST roundtrip 1.2e-15
SHTPlan sphtor == non-plan sphtor 4.4e-16
SHqst_to_point == the grid it came from 2.2e-16
packed_cplx == dense synthesis 0.0e+00
analysis_sphtor(Pencil) == (Matrix) 5.8e-17
Serial 67278/67278; 8 MPI suites at 4 ranks; 56 custom assertions at 8 ranks.
GPU sphtor change is NOT runtime-verified -- no CUDA device here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
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.
No description provided.