-
-
Notifications
You must be signed in to change notification settings - Fork 629
fix(Mpolynomial_libsingular): support division over transcendental ex… #40076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix(Mpolynomial_libsingular): support division over transcendental ex… #40076
Conversation
43fadd2
to
c1cc3c7
Compare
you need to add a doctest showing what has been fixed |
c1cc3c7
to
161c2a9
Compare
CI fails (e.g. https://github.com/sagemath/sage/actions/runs/14929279618/job/41946713381?pr=40076) with
|
The first two CI failures (for _div_ and _floordiv_) appears to be caused by meson using Singular 4.4.1 from conda-forge instead of building from source with the patch. I don't know how to fix this part as it doesn't seem like the meson build system is hooked into I am not sure about the cause of the third error on factor. |
10698a2
to
55259bf
Compare
We now have Singular 4.4.1 in the develop branch, so you'll need to update this. |
55259bf
to
d866b47
Compare
Thanks for the heads-up, I have rebased on top of the develop branch and squashed the fixup commit. |
…tension Currently, dividing a Singular backed multivariate polynomial by a fractional field element(of the same type) is unsupported and triggers a conversion error on the Singular end indicating the denominator is nonconstant. This commit extracts the commit from Singular trunk which adds support for the use case[^1] to a patch to be applied on top of release 4.4.1. We force an in-source build of Singular if the system Singular package does not contain the patch. [^1]: Singular/Singular@0a4bc0e
d866b47
to
fb8d1c5
Compare
There still seems to be some mismatches with sonames. I am not very familiar with the sage build system, any pointers on how to resolve these would be greatly appreciated. |
do you mean, in CI logs? |
I see. I have tested the changes locally and addressed the previous comments. There is one doctest failure I got when testing the changes to
I am not sure why the expected result is Is there anything else that I need to work on to get the PR to a mergeable state? |
OK, so this doctest should be fixed. Please don't just correct the result, but add some text explaining why this is true. |
Understood, should I open another PR for the doctest fix? |
why - this one is still open, do it here |
…d output This commit updates the expected output for one failing doctest under the _floordiv_ function, namely the test for floor division over large prime characteristic fields. The offending test computes ((x+y)^3+x+z)//(x+y) with the expected answer x^2 + 2*x*y + y^2 under the default grevlex order. However the actual quotient should be x^2 + 2*x*y + y^2 + 1 as computed by Macaulay2 and verified by hand calculation.
Ok, I thought it would be beneficial to track these changes separately since they are unrelated. I have just appended a commit fixing the doctest. |
if this test result changes as a result of this PR changes, then it should be updated here. Otherwise it's up to you. |
Documentation preview for this PR (built with commit d6eb1e0; changes) is ready! 🎉 |
…tension
Currently, dividing a Singular backed multivariate polynomial by a fractional field element(of the same type) is unsupported and triggers a conversion error on the Singular end indicating the denominator is nonconstant. This commit extracts the commit from Singular trunk which adds support for the use case1 to a patch to be applied on top of release 4.4.1.
fixes #39801
Relevant discussions on sage-devel: https://groups.google.com/g/sage-devel/c/Npn48ofmHF4
I have opted to not add doctests due to anticipated difficulty in coordinating adoption of the patch across different distribution channels. Notwithstanding this, I still think doctests should be added at some point to guard against regression. If this is important enough to warrant adding the relevant tests now, please let me know. Otherwise, I think the tests can be added after the next Singular release whenever that occurs.
This is my first time contributing to sage. It is possible I missed some details in the developer documentation and contributing guidelines. Please advise me if I am doing anything unidiomatic or incorrect.
📝 Checklist
⌛ Dependencies
#40033 - potential compatibility issues/breakage with Singular release 4.4.1 should be sorted out first
Footnotes
https://github.com/Singular/Singular/commit/0a4bc0e2888e660d1a5229a7b4842539a46c912a ↩