-
-
Notifications
You must be signed in to change notification settings - Fork 588
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
make polynomial truncation cpdef method #4406
Comments
comment:1
Attachment: 4406-cpdef-truncate.patch.gz This wasn't as invasive as I had expected. Apply on top of fix at #2462. |
comment:2
Patch looks good to me, but I will wait on a review #2462 before testing this. Also fixed a spelling mistake in the subject. Cheers, Michael |
comment:3
This patch causes the following doctest failures:
The error seems to always be
Cheers, Michael |
comment:4
Are you sure this is with my patch? I just tried these and they work fine in my branch. Or maybe it's some incompatibility with your alpha. |
comment:5
Replying to @robertwb:
Yes, I tried with this and the patch at #2462 and initially suspected #2462, but it turns out to be this patch. Reverting this patch only fixed the issue for me. 3.2.alpha2 is coming today, so there should be a binary for sage.math shortly. Cheers, Michael |
comment:6
OK, I'll look at it there. |
comment:7
I tried fixing this on sage.math, but I'm having issues with the unpacked tar. I copied over sage-3.2.alpha2-sage.math-only-x86_64-Linux and extracted it, but when I run ./sage I get
and I can't figure out how test my changes. However, I'm pretty sure the error is because line 467 of sage/rings/polynomial/polynomial_template.pxi is still def (rather than cpdef). I'm attaching a patch that should fix the problem. |
Attachment: 4406-truncate-fix.patch.gz |
comment:9
I will test the patch and see if that fixes it. More than likely you are getting bitten by #4317, so following the instructions there you should be able to fix the problem. Cheers, Michael |
comment:10
The second patch Robert posted resolves the issue I found. Positive review. Cheers, Michael |
comment:11
Merged in Sage 3.2.alpha3 |
Currently we have _c variants, some of which call one direction, and some which call the other.
CC: @craigcitro
Component: algebra
Issue created by migration from https://trac.sagemath.org/ticket/4406
The text was updated successfully, but these errors were encountered: