Fix Jasp quantum-quantum QuantumModulus multiplication in QuantumArray - #710
Open
alighazi288 wants to merge 1 commit into
Open
Fix Jasp quantum-quantum QuantumModulus multiplication in QuantumArray#710alighazi288 wants to merge 1 commit into
alighazi288 wants to merge 1 commit into
Conversation
Element-wise quantum-quantum multiplication of QuantumArrays of QuantumModulus returned a value scaled by a power of two in dynamic (Jasp) mode, e.g. 1 * 5 mod 7 gave 3 instead of 5 (see eclipse-qrisp#640). qq_montgomery_multiply_modulus computed the Montgomery reduction shift as smallest_power_of_two((N-1)^2 + 1) - n, whereas the static montgomery_mod_mul and create_output_qf use ceil(log2((N-1)^2) + 1) - n. Because ceil(log2(k^2 + 1)) differs from ceil(log2(k^2)) + 1, the jasp result's res.m came out one too small for many moduli (e.g. 7, 11, 13). A single QuantumModulus product returns that result directly and still decodes correctly, but the QuantumArray element-wise path preallocates the output's Montgomery shift via create_output_qf, so the injected result was decoded against a mismatching shift and came out off by 2^1. Align the jasp shift with the static formula (JAX/BigInteger-safe: the "+ 1" moves outside smallest_power_of_two) and unskip the existing QuantumModulus quantum-quantum multiplication test. Closes eclipse-qrisp#640 Signed-off-by: Syed Ali Ghazi <alighazi0609@gmail.com>
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
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.
Description
Fixes element-wise quantum-quantum multiplication of
QuantumArrays ofQuantumModulusin dynamic (Jasp) mode, which returned a value scaled by a power of two. e.g.1 * 5 mod 7gave3instead of5.Root cause:
qq_montgomery_multiply_moduluscomputed the Montgomery reduction shift assmallest_power_of_two((N-1)**2 + 1) - n, whereas the staticmontgomery_mod_mulandcreate_output_qfuseceil(log2((N-1)**2) + 1) - n. Sinceceil(log2(k²+1)) ≠ ceil(log2(k²)) + 1, the jasp result'sres.mcame out one too small for many moduli (7, 11, 13, …). A singleQuantumModulusproduct returns that result directly and still decodes correctly, but theQuantumArrayelement-wise path preallocates the output's Montgomery shift viacreate_output_qf, so the injected result was decoded against a mismatching shift and came out off by2¹. The fix aligns the jasp shift with the static formula (JAX/BigInteger-safe — the+ 1moves outsidesmallest_power_of_two).Related Issues
Closes #640
Type of Change
Breaking Change?
What was changed?
qq_montgomery_multiply_modulus(smallest_power_of_two((N-1)**2 + 1) - n→smallest_power_of_two((N-1)**2) + 1 - n) so it matches the staticmontgomery_mod_mul/create_output_qfconvention, with a comment documenting the invariant.pytest.skipguarding theQuantumModulusquantum-quantum cases intest_quantum_array_element_wise_ops_qm, so they now run.How was it tested?
a * belement-wise onQuantumArray[QuantumModulus]returns(A*B) % N- issue repro (1*5 mod 7 = 5) + random 2×2/3×2 arrays over N ∈ {7,11,13,15,23,31,47}QuantumArrayof non-QuantumModulusstill raisesNotImplementedError(unchanged)m=0) and pre-shifted (m≠0) single-QuantumModulusproducts remain correct; smallest modulus exercised via scalar-RHS quantum case (N=3)(a*b)*cand non-square arrays across several moduliLocal suites green:
test_jasp_quantum_array.py(113),test_jasp_modulus.py(56, incl. the qq standard-form / non-standard-m/ traced-BigInteger regressions),test_montgomery.py. Also confirmed the corrected shift equals the static formula for every odd modulus 3–63.Checklist
Reviewer Notes
The previously-skipped
test_quantum_array_element_wise_ops_qm(mul, quantum RHS) is now un-skipped and serves as the regression guard (its N=7 array case reproduces the bug).Design note: this aligns the jasp shift with the existing canonical formula, which makes it one larger for affected moduli so
qq_montgomery_multiply_modulususes ~1 extra ancilla qubit, matching what the static path already allocates. I chose this fix over the qubit-optimal alternative (keep the tighter jasp shift and instead have the injection carry the actualres.m), which would touch the well-testedcreate_output_qf/injection machinery. Happy to go the other route if you'd prefer.