Improve counting of local solutions for QuadraticForm at p=2 #38680
Conversation
| @@ -228,27 +228,27 @@ cdef local_solution_type_cdef(Q, p, w, zvec, nzvec): | |||
| i += 1 | |||
|
|
|||
| if not nonzero_flag: | |||
| return <long> 0 | |||
| return < long > 0 | |||
There was a problem hiding this comment.
this is not wanted, keep <long>
|
|
||
| # Error if we get here! =o | ||
| print(" Solution vector is " + str(w)) | ||
| print(" and Q is \n" + str(Q) + "\n") | ||
| raise RuntimeError("Error in IsLocalSolutionType: Should not execute this line... =( \n") | ||
|
|
||
|
|
||
| def CountAllLocalGoodTypesNormalForm(Q, p, k, m, zvec, nzvec): |
There was a problem hiding this comment.
do not use CamelCase name for functions
|
Documentation preview for this PR (built with commit b945f4b; changes) is ready! 🎉 |
|
one failing doctest |
The old doctest failed the input assumption that Q is given in local diagonal form. Now gives such a local diagonal form to the function. The new output has also been verified with the old code.
|
Should be resolved now. The old doctest failed the (already existing) input assumption that Q is given in local diagonal form. The new code uses this assumption and therefore gave unexpected results. Now the local diagonal form is passed to the function in the doctest. I verified the results also with release version 10.4. |
|
The previous build resulted in an error because some .coverage file was missing (see https://github.com/sagemath/sage/actions/runs/10962048022/job/30442695828 ). I also merged the latest develop branch again, maybe this will already resolve the issue. |
|
That seems to have fixed it, should be good to merge now. |
|
the documentation in |
| - ``Q`` -- quadratic form over `\ZZ` in local normal form at p with no zero blocks mod m_range | ||
| - ``p`` -- prime number > 0 | ||
| - ``k`` -- integer > 0 | ||
| - ``m`` -- integer >= 0 (depending only on mod `p^k`) | ||
| - ``zvec``, ``nzvec`` -- list of integers in ``range(Q.dim())``, or ``None`` | ||
|
|
||
| OUTPUT: | ||
|
|
||
| an integer '\ge 0' representing the solutions of Good type. |
There was a problem hiding this comment.
A few things here:
- If I see it correctly, m_range is always p^k here, so it might be nicer to write "mod
p^k" in the first line - In the output line '\ge 0' is not displaying properly since the wrong ticks are used; it should be
\ge 0, or alternatively you could also write "a non-negative integer" instead (I think the latter is slightly better, but either should be fine) - Not sure if it is common to say "representing the solutions" for the count of solutions, but maybe "giving the number of solutions" would be a bit more explicit?
|
Thank you both for noticing this and for the suggestions. I've updated the docs accordingly. |
Thanks for the changes, looks good to me now! Not sure why the workflows require approval, but since the last commit only changed documentation and the tests ran correctly before, I'll set this to positive review. |
…rm at p=2
Fixes sagemath#38679.
Introduces new method `CountAllLocalGoodTypesNormalForm(Q, p, k, m,
zvec, nzvec)` to compute the number of local solutions of 'good type'
for Q[v] = m mod p^k when Q is in local normal form at p.
This replaces the use of the slow brute-force function
`CountAllLocalTypesNaive` indirectly used in
`local_good_density_congruence_even` for (p,k)=(2,3) .
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
### ⌛ Dependencies
URL: sagemath#38680
Reported by: WvanWoerden
Reviewer(s): Frédéric Chapoton, Sebastian A. Spindler
…rm at p=2
Fixes sagemath#38679.
Introduces new method `CountAllLocalGoodTypesNormalForm(Q, p, k, m,
zvec, nzvec)` to compute the number of local solutions of 'good type'
for Q[v] = m mod p^k when Q is in local normal form at p.
This replaces the use of the slow brute-force function
`CountAllLocalTypesNaive` indirectly used in
`local_good_density_congruence_even` for (p,k)=(2,3) .
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
### ⌛ Dependencies
URL: sagemath#38680
Reported by: WvanWoerden
Reviewer(s): Frédéric Chapoton, Sebastian A. Spindler
…rm at p=2
Fixes sagemath#38679.
Introduces new method `CountAllLocalGoodTypesNormalForm(Q, p, k, m,
zvec, nzvec)` to compute the number of local solutions of 'good type'
for Q[v] = m mod p^k when Q is in local normal form at p.
This replaces the use of the slow brute-force function
`CountAllLocalTypesNaive` indirectly used in
`local_good_density_congruence_even` for (p,k)=(2,3) .
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
### ⌛ Dependencies
URL: sagemath#38680
Reported by: WvanWoerden
Reviewer(s): Frédéric Chapoton, Sebastian A. Spindler
Fixes #38679.
Introduces new method
CountAllLocalGoodTypesNormalForm(Q, p, k, m, zvec, nzvec)to compute the number of local solutions of 'good type' for Q[v] = m mod p^k when Q is in local normal form at p.This replaces the use of the slow brute-force function
CountAllLocalTypesNaiveindirectly used inlocal_good_density_congruence_evenfor (p,k)=(2,3) .📝 Checklist
⌛ Dependencies