Allow more rings to be used with libsingular & rework signal detection system #39075
Conversation
This file contains 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
user202729
force-pushed
the
libsingular-more-rings
branch
from
acfbf30 to
2e88a31
Compare
|
Documentation preview for this PR (built with commit 963dde6; changes) is ready! 🎉 |
user202729
commented
Dec 3, 2024
|
|
||
| sage: P.<x,y,z> = PolynomialRing(Integers(2^32), order='lex') | ||
| sage: P(2^32-1) | ||
| 4294967295 | ||
| -1 |
There was a problem hiding this comment.
This is harmless, but technically modifies the behavior. (New behavior is probably better though.)
user202729
commented
Dec 3, 2024
| @@ -4527,8 +4527,7 @@ def groebner_basis(self, algorithm='', deg_bound=None, mult_bound=None, prot=Fal | |||
| sage: R.<x,y,z> = PolynomialRing(Zmod(2233497349584)) | |||
| sage: I = R.ideal([z*(x-3*y), 3^2*x^2-y*z, z^2+y^2]) | |||
| sage: I.groebner_basis() | |||
| [2*z^4, y*z^2 + 81*z^3, 248166372176*z^3, 9*x^2 - y*z, y^2 + z^2, x*z + | |||
| 2233497349581*y*z, 248166372176*y*z] | |||
| [2*z^4, y*z^2 + 81*z^3, 248166372176*z^3, 9*x^2 + 2233497349583*y*z, y^2 + z^2, x*z + 2233497349581*y*z, 248166372176*y*z] | |||
There was a problem hiding this comment.
This time around the new behavior is worse. Thoughts?
p/s. See also #39018
|
Should be fixed now. I clean up the code:
Looks like Side note: I don't think correct handling of ctrl-c work yet. |
|
Alright, ctrl-c should works properly now. Nevertheless, note that most interruptibility tests in SageMath are not that useful — those which are useful must run >10 minutes if not interrupted; in other words, the tests with In some cases, I delete checks like Related: my write-up on Let's see if the tests pass. p/s there's no interrupt test for |
user202729
changed the title
|
The failure is sage -t --long --warn-long 30.0 --random-seed=286735480429121101562228604801325644303 src/sage/rings/polynomial/polynomial_element.pyx # Timed outLooks irrelevant. Also I can't reproduce it locally. On the other hand the Conda 3.11 exception is LogIt does pertain to singular, but looks like it comes from the line maybe the library is somehow just nonexistent on Conda? (weird?) The Conda 3.9 exception is Logwhich… is pretty weird because as far as I can see all route leads to that line must have filled in that attribute already. → → Unless one of the following lines can error out. (my best guess is Edit: actually thinking about it the test server is passing |
|
Maybe @mantepse would be interested in reviewing? |
mantepse
reviewed
Dec 7, 2024
mantepse
reviewed
Dec 7, 2024
mantepse
reviewed
Dec 7, 2024
| _res = self.call_handler.handle_call(argument_list, si_ring) | ||
| sig_off() | ||
| finally: | ||
| s = check_error() |
There was a problem hiding this comment.
maybe a more descriptive name, e.g., e, or error, instead of s would be good?
mantepse
reviewed
Dec 7, 2024
Comment on lines
1493
to
1494
| if currentVoice: | ||
| currentVoice = NULL |
There was a problem hiding this comment.
I could not find any use of currentVoice within sage, do you know what this is? (It does appear in local/include/singular/Singular/sdb.h and local/include/singular/Singular/fevoices.h)
There was a problem hiding this comment.
No idea, but given the naming, my best guess is "who is speaking to Singular" i.e. "where is Singular taking commands from". The example has currentVoice=feInitStdin(NULL); (read: stdin) and we certainly don't want it to read from stdin.
mantepse
reviewed
Dec 7, 2024
|
I'm very sorry, but it seems that I somehow mistested my code. I have no idea what's going wrong, but I guess the best way is to reinstate the old version. Perhaps cython's support of diff --git a/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx b/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
index 423b5c0e603..adc8f503f6c 100644
--- a/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
+++ b/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
@@ -4579,8 +4579,9 @@ cdef class MPolynomial_libsingular(MPolynomial_libsingular_base):
raise NotImplementedError("Factorization of multivariate polynomials over prime fields with characteristic > 2^29 is not implemented.")
ivv = iv.ivGetVec()
- v = [(f, ivv[i]) for i in range(1, I.ncols)
- if (f := new_MP(parent, p_Copy(I.m[i], _ring))) != 0]
+ v = [(new_MP(parent, p_Copy(I.m[i], _ring)), ivv[i])
+ for i in range(1, I.ncols)]
+ v = [(f, m) for f, m in v if f != 0] # we might have zero in there
unit = new_MP(parent, p_Copy(I.m[0], _ring))
F = Factorization(v, unit) |
|
Time to report bug to Cython I guess. |
Co-authored-by: Martin Rubey <axiomize@yahoo.de>
user202729
force-pushed
the
libsingular-more-rings
branch
from
6babf2c to
55b698f
Compare
Wow, you are quick. For reference: cython/cython#6546 |
mantepse
approved these changes
Dec 8, 2024
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 15, 2024
…rk signal detection system
Somewhat related: sagemath#33319
Anyway, this allows many more rings to be used with libsingular.
Previously it fallback to the expect interface.
It seems weird that both Sage and Singular has different types for
`GF(5)` and `Zmod(5)`, but okay.
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [x] 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.
- [ ] I have updated the documentation and checked the documentation
preview.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
URL: sagemath#39075
Reported by: user202729
Reviewer(s): Martin Rubey, user202729
|
I'm getting (possibly computer too fast? Ryzen 9900X) |
github-actions
bot
added
s: needs review
and removed
s: positive review
s: needs work
labels
|
Sounds about right. I already made the buffer 2× on my computer though. Will make it 4×. |
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.
Somewhat related: #33319
Anyway, this allows many more rings to be used with libsingular. Previously it fallback to the expect interface.
It seems weird that both Sage and Singular has different types for
GF(5)andZmod(5), but okay.📝 Checklist
⌛ Dependencies